-
Notifications
You must be signed in to change notification settings - Fork 417
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
Conversation
Question for maintainers, what do you prefer:
|
d1f4e3f
to
09940c9
Compare
… std in other places Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
09940c9
to
ac76951
Compare
Option 1 looks good to me, as there could be an inevitable scenario where users would have taken a dependency on nostd::type_traits. |
There was a problem hiding this 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?
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 |
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 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 So, in my opinion, option "3 - doing nothing ... for now" is the only valid choice. |
There was a problem hiding this 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"
It's just fine to remove BTW: Another useful API only in C++14 is |
As commented previously:
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. |
Closing. To re evaluate when @owent no longer needs the code to still build with C++11. |
The API package already uses std in other places.