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

Need fine-grained HAVE_CPP_STDLIB #1071

Closed
hcoona opened this issue Nov 16, 2021 · 10 comments · Fixed by #2304
Closed

Need fine-grained HAVE_CPP_STDLIB #1071

hcoona opened this issue Nov 16, 2021 · 10 comments · Fixed by #2304
Assignees
Labels
do-not-stale enhancement New feature or request

Comments

@hcoona
Copy link
Contributor

hcoona commented Nov 16, 2021

Is your feature request related to a problem?

The std::shared_ptr & std::unique_ptr are well-supported by wide-spread compilers in c++11, while std::string_view & std::variant are unavailable until c++17. We currently use a coarse-grained switch HAVE_CPP_STDLIB to control all these things. As a result, we cannot enable any of them.

Using our private implemented nonstd::shared_ptr would lead to interoperability issue with std::shared_ptr, which is quite annoying.

Describe the solution you'd like

I'd like to split HAVE_CPP_STDLIB into more fine-grained switches:

  • OPENTELEMETRY_HAVE_CXX11
    • OPENTELEMETRY_HAVE_STD_SHARED_PTR
    • OPENTELEMETRY_HAVE_STD_UNIQUE_PTR
  • OPENTELEMETRY_HAVE_CXX17
    • OPENTELEMETRY_HAVE_STD_STRING_VIEW
    • OPENTELEMETRY_HAVE_STD_VARIANT
  • OPENTELEMETRY_HAVE_CXX20
    • OPENTELEMETRY_HAVE_STD_SPAN

Describe alternatives you've considered

Didn't find a better approach.

@hcoona
Copy link
Contributor Author

hcoona commented Nov 16, 2021

I think the switches could be smarter to automatically detect the value from built-in macros & pragmas.

And we provide some switches for user overridings.

  • 0: Automatically
  • 1: Force STD
  • 2: Force Non-STD
  • 3: Force Abseil-cpp
  • 4: Force GSL

I personally would prefer STD->Abseil/GSL->Non-STD.

@lalitb
Copy link
Member

lalitb commented Nov 16, 2021

I personally would prefer STD->Abseil/GSL->Non-STD.

It's encouraged to always use non-std libraries to maintain ABI compatibility ( https://github.com/open-telemetry/opentelemetry-cpp/blob/main/docs/abi-policy.md#application-binary-interface-abi-policy ). While there are macros switches provided to use other libraries, their use is discouraged. These switches were provided as in the past there were issues reported during compilation with these non-std, but I believe they all should be fixed now.

@lalitb
Copy link
Member

lalitb commented Nov 16, 2021

We currently use a coarse-grained switch HAVE_CPP_STDLIB to control all these things. As a result, we cannot enable any of them.

Do we have a use case why do we want to enable the STL components individually?

@hcoona
Copy link
Contributor Author

hcoona commented Nov 16, 2021

  1. I have to do tedious convertions between std::string_view and nonstd::string_view. (actually absl::string_view in my case, because we are using c++14)
  2. nonstd::shared_ptr and nonstd::unique_ptr break the strict aliasing assumption, which prevent enable strict aliasing optimization in our code. (
    return *reinterpret_cast<shared_ptr_wrapper *>(buffer_.data);
    )

@hcoona
Copy link
Contributor Author

hcoona commented Nov 16, 2021

It's good to maintain ABI compatibility. If we provide such switches, we could still make the default setting ABI compatible.

@lalitb lalitb added enhancement New feature or request good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers and removed good first issue Good for newcomers labels Nov 17, 2021
@ThomsonTan
Copy link
Contributor

For the suggested flag like OPENTELEMETRY_HAVE_STD_SHARED_PTR, could they be detected by CMake automatically so no need to expose these flags to developers?

@owent
Copy link
Member

owent commented Jan 4, 2022

For the suggested flag like OPENTELEMETRY_HAVE_STD_SHARED_PTR, could they be detected by CMake automatically so no need to expose these flags to developers?

check_cxx_source_compiles(
  "
#include <iostream>
#include <string_view>
int main() {
constexpr std::string_view unicode[] {\"▀▄─\", \"▄▀─\", \"▀─▄\", \"▄─▀\"};

for (int y{}, p{}; y != 6; ++y, p = ((p + 1) % 4)) {
  for (int x{}; x != 16; ++x)
    std::cout << unicode[p];
  std::cout << std::endl;
}
return 0;
}"
  LIBATFRAME_UTILS_TEST_STL_STRING_VIEW)

We use these cmake scripts to test std::string_view in our project, maybe we can also use it to test std::string_view, std::variant or std::span here?

With C++20 and upper, we can also use #if defined(__cpp_lib_variant) , #if defined(__cpp_lib_string_view) and #if defined(__cpp_lib_span) (https://en.cppreference.com/w/cpp/feature_test) . But I have no idea about how to approach these tests with bazel.

BTW: I think all the environment thaa supported by opentelemetry-cpp has std::shared_ptr and std::unique_ptr , but not all the environments have std::make_span. Is it necessary to test std::shared_ptr or std::unique_ptr ?

@github-actions
Copy link

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.

@marcalff
Copy link
Member

marcalff commented Nov 2, 2022

Trying to use opentelemetry-cpp on several platforms, which have varying degrees of STL support, I am facing the same issues.

Currently:

  • HAVE_CPP_STDLIB is coarse grained, making it impossible to use some C++11/14/17 features without a full C++20 STL.
  • HAVE_CPP_STDLIB pollutes the application namespace, a OPENTELEMETRY prefix is needed.

Proposal for a fix:

In CMake, change the option to:

  • WITH_STL=OFF (unchanged). Only setting that guarantees ABI stability.
  • WITH_STL=CXX11 (new)
  • WITH_STL=CXX14 (new)
  • WITH_STL=CXX17 (new)
  • WITH_STL=CXX20 (new)
  • WITH_STL=ON (unchanged, means WITH_STL=CXX20 as currently)

Use pre processor symbols like:

  • OPENTELEMETRY_HAVE_CXX11
  • OPENTELEMETRY_HAVE_CXX14
  • OPENTELEMETRY_HAVE_CXX17
  • OPENTELEMETRY_HAVE_CXX20

When a feature is not available in the STL library, for example when building WITH_STL=CXX17, alternate implementations needed to satisfy nostd can be chosen depending on WITH_GSL, as currently for nostd::span.

For example,

Building with WITH_STL=CXX17 will define the following symbols:

  • OPENTELEMETRY_HAVE_CXX11
  • OPENTELEMETRY_HAVE_CXX14
  • OPENTELEMETRY_HAVE_CXX17

which means that:

  • unique_ptr, shared_ptr will use the STL (available since C++11)
  • index_sequence will use the STL (available since C++14)
  • string_view and variant will use the STL (available since C++17)

but nostd::span will not use the STL (available in C++20 only)

As for individual flags, like OPENTELEMETRY_HAVE_STD_SHARED_PTR, I don't see the need for them.

The assumption is that if a STL implement all C++11 features, there should be no need to pick STD_SHARED_PTR independently from STD_UNIQUE_PTR. Same for any STL flavor.

@marcalff marcalff self-assigned this Nov 2, 2022
@lalitb lalitb removed the help wanted Good for taking. Extra help will be provided by maintainers label Nov 7, 2022
@marcalff marcalff added this to the Migrate to C++14 milestone May 23, 2023
@malkia
Copy link

malkia commented May 23, 2023

I had to declare HAVE_CPP_STDLIB top-level for projects using OpenTelemetry, and wished it was prefixed with OPENTELEMETRY_, and then split if needed (suggested above). So I would love to see this coming - OPENTELEMETRY_HAVE_CXXnn - then it'll make sense for us to have these declared top-level somewhere, otherwise it seems unclean (unlikely to break things for us, but who knows if another OSS project usese HAVE_CPP_STDLIB)

marcalff added a commit to marcalff/opentelemetry-cpp that referenced this issue Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-stale enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants