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

Use std shared_ptr, unique_ptr and enable_if #2425

Closed
wants to merge 1 commit into from

Conversation

bogdandrutu
Copy link
Member

Fixes #1877

@bogdandrutu bogdandrutu requested a review from a team December 2, 2023 00:28
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

I believe the correct approach is to use nostd:: constructs in the SDK when implementing the Otel API interfaces, as specified in their interface declarations.

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.

As noted already by @lalitb,
when a virtual method returns nostd::unique_ptr in the API,
the SDK implementation should also use nostd: the SDK can not change the return type, this is what is causing build failures currently, for example for GetMeter().

For code which is purely internal, and for code exposed only by the SDK or exporters, like global_log_handler.h for example, I think it is ok to use std classes, however there is this issue:

When designing std::unique_ptr<T> do_something_internal(),
it is very challenging to know, in advance, if the result of this call will eventually (or never) by returned all the way up to the API, which requires to either convert std to nostd, or change the code to use nostd in do_something_internal().

On a totally different topic, std::enable_if is about meta-programming, and never shows up in the ABI.

I think it should be safe to replace nostd::enable_if with std::enable_if, independently of what we decide for unique_ptr and shared_ptr.

According to https://en.cppreference.com/w/cpp/types/enable_if:

  • std::enable_if is available since C++11
  • std::enable_if_t is available since C++14

and we now require C++14,
so both should be usable.

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.

Inconsistency between the use of std::shared_ptr and nostd::shared_ptr
3 participants