opentelemetry-cpp icon indicating copy to clipboard operation
opentelemetry-cpp copied to clipboard

Remove unnecessary usages of nostd::type_traits in API.

Open bogdandrutu opened this issue 1 year ago • 5 comments
trafficstars

The API package already uses std in other places.

bogdandrutu avatar Feb 03 '24 21:02 bogdandrutu

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.

bogdandrutu avatar Feb 03 '24 21:02 bogdandrutu

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

lalitb avatar Feb 05 '24 20:02 lalitb

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

ThomsonTan avatar Feb 08 '24 18:02 ThomsonTan

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.

marcalff avatar Feb 17 '24 22:02 marcalff

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.

owent avatar Feb 18 '24 03:02 owent

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 avatar Mar 03 '24 21:03 marcalff

Closing.

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

marcalff avatar Mar 03 '24 21:03 marcalff