Skip to content

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

5 participants