Skip to content

Conversation

georgthegreat
Copy link

@georgthegreat georgthegreat commented Apr 2, 2025

Fixes # (issue)

Changes

Upon updating to 1.20.0 we have encountered a bunch of the following errors:

contrib/libs/opentelemetry-cpp/api/include/opentelemetry/nostd/type_traits.h:145:73: error: builtin __has_trivial_copy is deprecated; use __is_trivially_copyable instead [-Werror,-Wdeprecated-builtins]
  145 |   static constexpr bool value = std::is_copy_constructible<T>::value && __has_trivial_copy(T);

(we use clang-18 to build our code).
It turned out that some old compatibility code got broken.

@marcalff, according to README.md opentelemetry-cpp does no longer support standards below C++11, hence we can always use these type_traits from the standard library.
Can we drop the C++03 compat and simplify the things even further?

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes

@georgthegreat georgthegreat requested a review from a team as a code owner April 2, 2025 09:49
@netlify
Copy link

netlify bot commented Apr 2, 2025

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit 14e036a
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/67f556c01b44cd0008167db8

@codecov
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.56%. Comparing base (b6908f0) to head (e081139).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3337      +/-   ##
==========================================
+ Coverage   89.55%   89.56%   +0.02%     
==========================================
  Files         210      210              
  Lines        6502     6502              
==========================================
+ Hits         5822     5823       +1     
+ Misses        680      679       -1     

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@marcalff marcalff self-assigned this Apr 2, 2025
@marcalff
Copy link
Member

marcalff commented Apr 2, 2025

@georgthegreat Thanks for the PR.

Concerning the issue, we also use clang-18 in our CI, and this never broke this way.
I really would like to understand the root cause of this, could you clarify what are the values for:

  • the C++ standard version used at compile time (C++11, 14, 17, ...)
  • The build system used: CMake, bazel, plain makefiles, ...
  • If this breaks with compiling opentelemetry-cpp itself, or when compiling an application using it
  • If OPENTELEMETRY_STL_VERSION is defined in the build, and to what value
  • If OPENTELEMETRY_TRIVIALITY_TYPE_TRAITS is defined (I assume it was not, please confirm)

Concerning the fix, I agree it should be simplified.
I will add review comments to make this even simpler.

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.

Thanks for the PR.

See suggestion to simplify even more.

@marcalff
Copy link
Member

marcalff commented Apr 2, 2025

Seem also related to WITH_STL, we need to clarify whether to test __cplus_plus or OPENTELEMETRY_STL_VERSION to check for C++11.

@marcalff
Copy link
Member

marcalff commented Apr 2, 2025

@georgthegreat
Copy link
Author

@marcalff, we use clang-18, -std=c++20 and -DOPENTELEMETRY_STL_VERSION=2023 (I see the divergence here but I don't see how it could lead to the issue).

I have removed OPENTELEMETRY_TRIVIALITY_TYPE_TRAITS as you suggested.

@georgthegreat
Copy link
Author

@marcalff let's merge it.

* std::is_trivialy_move_assignable
*/
#ifdef OPENTELEMETRY_TRIVIALITY_TYPE_TRAITS
#if defined(OPENTELEMETRY_STL_VERSION) && OPENTELEMETRY_STL_VERSION >= 2011
Copy link
Member

Choose a reason for hiding this comment

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

The fallback implementation relies on GCC's API, making it exclusive to GCC. It should be updated to support all compilers.

Copy link
Author

Choose a reason for hiding this comment

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

Why should it? Opentelemetry does not support cxx03 standard.

Copy link
Member

Choose a reason for hiding this comment

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

I mean the code in #else branch

Copy link
Author

Choose a reason for hiding this comment

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

This is a dead code, opentelemetry does not support cxx03 standard / libstdc++ 5.

Copy link
Member

Choose a reason for hiding this comment

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

@georgthegreat

In theory, yes, this is dead code.
In practice, no, this code is not totally dead now, there are historical reasons to maintain this part working.

What we need here is a fix that solves the trivial cases of building with recent compilers, using std:: type traits when they are available, instead of using the alternate nostd:: implementation.

The issue is probably that we need to do this independently of WITH_STL : that is, test __cplusplus instead of OPENTELEMETRY_STL_VERSION.

Now, for old cases when std:: classes are not available, like this old work around for gcc 4.8, the code needs to implement the work around as before.

Copy link
Author

Choose a reason for hiding this comment

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

I hope I understood you correctly.

Copy link
Member

Choose a reason for hiding this comment

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

#if !defined(OPENTELEMETRY_HAVE_STD_TYPE_TRAITS) can be removed now.

#endif

#if !defined(OPENTELEMETRY_HAVE_STD_TYPE_TRAITS)
# include <array>
Copy link
Member

Choose a reason for hiding this comment

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

#if !defined(OPENTELEMETRY_HAVE_STD_TYPE_TRAITS) Can be removed, but these headers are still required.

@georgthegreat
Copy link
Author

I am done with it.
I definitely have more interesting work to do compared to supporting 12 year old compiler.

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.

4 participants