Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary usages of nostd::type_traits in API. #2522

Closed
wants to merge 1 commit into from

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Feb 3, 2024

The API package already uses std in other places.

@bogdandrutu bogdandrutu requested a review from a team February 3, 2024 21:49
@bogdandrutu
Copy link
Member Author

Question for maintainers, what do you prefer:

  1. Still keep nostd type_traits and always make them equivalent with std since we are on c++14.
  2. Remove nostd::type_traits form the API.
  3. Doing nothing.

… std in other places

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@lalitb
Copy link
Member

lalitb commented Feb 5, 2024

Option 1 looks good to me, as there could be an inevitable scenario where users would have taken a dependency on nostd::type_traits.

Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some old toolchains do not support C++14 libraries completely. Could we just keep the nostd version here?

@ThomsonTan
Copy link
Contributor

Some old toolchains do not support C++14 libraries completely. Could we just keep the nostd version here?

But C++ 14 is required for OpenTelemetry. Or which version of toolchain is still under concern?

https://github.com/open-telemetry/opentelemetry-cpp#supported-c-versions

@marcalff
Copy link
Member

Question for maintainers, what do you prefer:

1. Still keep nostd type_traits and always make them equivalent with std since we are on c++14.

2. Remove nostd::type_traits form the API.

3. Doing nothing.

Given that opentelemetry-cpp now officially does not support C++11, and requires C++14, the logical answer should be 2, and keeping a typedef from nostd::type_traits for std::type_traits to avoid breaking users who used the nostd flavor.

And I even suggested that myself (to remove nostd::type_traits) in a different PR.

There is more to this story however.

C++14 is officially supported, which means we aim to support any combination of API, SDK, exporters, libraries (GTest, benchmark, CURL, grpc, ...) with C++14, and want to maintain a working CI for this.

At the same time, C++11 is informally supported.

opentelemetry-cpp is still used, even with current versions, with C++11, with some well chosen combinations of exporters and third party libraries, and one known user still on C++11 is @owent

While we do not want to maintain a full CI for C++11, we (opentelemetry-cpp) should also attempt, if possible, to maintain a status quo and still have code that works in C++11, as a courtesy for a major contributor: @owent ranks 5th in terms of commits (82), and 2nd in terms of lines added (34,872), at time of writing.

Personally, I would love to see nostd::type_traits go away, but it looks we will have to keep some nostd parts a little longer, until the time when @owent no longer has the C++11 constraint.

So, in my opinion, option "3 - doing nothing ... for now" is the only valid choice.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup.

See discussion about options : "3 - doing nothing ... for now"

@owent
Copy link
Member

owent commented Feb 18, 2024

Question for maintainers, what do you prefer:

1. Still keep nostd type_traits and always make them equivalent with std since we are on c++14.

2. Remove nostd::type_traits form the API.

3. Doing nothing.

Given that opentelemetry-cpp now officially does not support C++11, and requires C++14, the logical answer should be 2, and keeping a typedef from nostd::type_traits for std::type_traits to avoid breaking users who used the nostd flavor.

And I even suggested that myself (to remove nostd::type_traits) in a different PR.

There is more to this story however.

C++14 is officially supported, which means we aim to support any combination of API, SDK, exporters, libraries (GTest, benchmark, CURL, grpc, ...) with C++14, and want to maintain a working CI for this.

At the same time, C++11 is informally supported.

opentelemetry-cpp is still used, even with current versions, with C++11, with some well chosen combinations of exporters and third party libraries, and one known user still on C++11 is @owent

While we do not want to maintain a full CI for C++11, we (opentelemetry-cpp) should also attempt, if possible, to maintain a status quo and still have code that works in C++11, as a courtesy for a major contributor: @owent ranks 5th in terms of commits (82), and 2nd in terms of lines added (34,872), at time of writing.

Personally, I would love to see nostd::type_traits go away, but it looks we will have to keep some nostd parts a little longer, until the time when @owent no longer has the C++11 constraint.

So, in my opinion, option "3 - doing nothing ... for now" is the only valid choice.

It's just fine to remove nostd::type_traits. What I want to do is use a small patch file to support C++11. I'm pushing teams to use new toolchains but it still need some time for told project to migrate.
I have another idea, can I add a compatible header to use nostd::type_traits to replace the type_traits in std namespace when using these old toolchains? It will leave the codes clean and just add a include sentense for each file include <type_traits>.

BTW: Another useful API only in C++14 is std::make_unique. I can also add it to this header.

@marcalff
Copy link
Member

marcalff commented Mar 3, 2024

As commented previously:

  • opentelemetry-cpp is officially supported for C++14 and up
  • some subset of opentelemetry-cpp is de facto still functional with C++11

There is some ambiguity between "supported in C++14" and "requiring C++14", and we would like to avoid actually requiring C++14, at least for some time, unless absolutely necessary.

Replacing nostd type traits with std type traits would require C++14 for no other gains.

This cleanup is valid, but postponed for now.

@marcalff
Copy link
Member

marcalff commented Mar 3, 2024

Closing.

To re evaluate when @owent no longer needs the code to still build with C++11.

@marcalff marcalff closed this Mar 3, 2024
@marcalff marcalff mentioned this pull request Oct 14, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants