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

Need fine-grained HAVE_CPP_STDLIB

Open hcoona opened this issue 3 years ago • 8 comments

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 avatar Nov 16 '21 12:11 hcoona

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.

hcoona avatar Nov 16 '21 12:11 hcoona

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 avatar Nov 16 '21 16:11 lalitb

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?

lalitb avatar Nov 16 '21 17:11 lalitb

  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. (https://github.com/open-telemetry/opentelemetry-cpp/blob/3db551f1b0026730c4b5227f1d683e1eb7b75dff/api/include/opentelemetry/nostd/shared_ptr.h#L144)

hcoona avatar Nov 16 '21 18:11 hcoona

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

hcoona avatar Nov 16 '21 18:11 hcoona

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?

ThomsonTan avatar Nov 17 '21 17:11 ThomsonTan

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 ?

owent avatar Jan 04 '22 09:01 owent

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

github-actions[bot] avatar Mar 16 '22 02:03 github-actions[bot]

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 avatar Nov 02 '22 14:11 marcalff

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)

malkia avatar May 23 '23 17:05 malkia