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

Fix compatibility when using clang and libc++ #1852

Merged
merged 3 commits into from
Dec 13, 2022

Conversation

owent
Copy link
Member

@owent owent commented Dec 8, 2022

Signed-off-by: owent [email protected]

Fixes #1851

Changes

  • Using fallback version of span when std::span do not support range.
  • Using cmake 3.20.6 in cmake.c++20.test and cmake.c++20.stl.test.(3.20.0 is the first version that support to add -std=c++20 to both gcc and clang.)
  • Remove the deprecated ATOMIC_FLAG_INIT usage when has __cpp_lib_atomic_value_initialization. (C++20)
  • Update GTest to 1.12.1 when using non-legacy toolchain.(The old version has -Werror and it will cause compiling error on modern compilers.)
  • Add CI job which using clang and -stdlib=libc++.

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

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@owent owent requested a review from a team December 8, 2022 03:46
@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #1852 (10cd536) into main (9f333ff) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 10cd536 differs from pull request most recent head b25f94e. Consider uploading reports for the commit b25f94e to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1852      +/-   ##
==========================================
- Coverage   85.79%   85.77%   -0.01%     
==========================================
  Files         171      171              
  Lines        5240     5240              
==========================================
- Hits         4495     4494       -1     
- Misses        745      746       +1     
Impacted Files Coverage Δ
api/include/opentelemetry/nostd/span.h 90.00% <ø> (ø)
...include/opentelemetry/sdk/trace/simple_processor.h 100.00% <ø> (ø)
sdk/src/metrics/meter_context.cc 88.89% <100.00%> (ø)
sdk/src/trace/batch_span_processor.cc 90.63% <0.00%> (-0.78%) ⬇️

@owent owent force-pushed the fix_1851 branch 2 times, most recently from c656ae4 to 9381bb7 Compare December 8, 2022 08:04
third_party_release Outdated Show resolved Hide resolved
@owent owent changed the title Fix compatibility when using clang and libc++ [WIP] Fix compatibility when using clang and libc++ Dec 8, 2022
@owent owent changed the title [WIP] Fix compatibility when using clang and libc++ Fix compatibility when using clang and libc++ Dec 9, 2022
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.

LGTM, thanks for the fix.

Since PR #1844 was merged here, please clarify how to merge to main:

  • Merge this change, and drop PR 1844 ?

@owent
Copy link
Member Author

owent commented Dec 9, 2022

LGTM, thanks for the fix.

Since PR #1844 was merged here, please clarify how to merge to main:

  • Merge this change, and drop PR 1844 ?

We can merge #1844 first, and rebase this PR from main again.

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.

LGTM. Nicely done. Thanks. Also, please fix the merge conflict so it can be merged.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
# include <type_traits>

/**
* @brief Clang 14.0.0 with libc++ do not support implicitly construct a span for a range.We just
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @brief Clang 14.0.0 with libc++ do not support implicitly construct a span for a range.We just
* @brief Clang 14.0.0 with libc++ do not support implicitly construct a span for a range. We just

Is this an issue for clang with libc++ 14.0.0?

Copy link
Member Author

@owent owent Dec 12, 2022

Choose a reason for hiding this comment

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

Yes, In my tests, clang with libc++ 14.0.1 do not has this problem, but if we install clang-14 through apt install clang-14 libc++-dev-14 libc++1-14 libc++abi-dev-14 libc++abi1-14 on Ubuntu 22.04, it will install 14.0.0 now(I run apt update on 2022-12-09).

@@ -89,11 +89,13 @@ struct Uri
class NoopEventHandler : public http_client::EventHandler
{
public:
void OnEvent(http_client::SessionState state, nostd::string_view reason) noexcept override {}
void OnEvent(http_client::SessionState /* state */,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this introduced to workaround some compilation warning? Wrap the parameter name in inline comment seems more verbose than the original parameter there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, clang will trigger -Werror,-Wunused-parameter when compiling with -Wextra -Werror.

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.

Reviewed again after the latest merge.

Using env: in ci.yml is much better, thanks for the cleanup.

LGTM.

Signed-off-by: owent <[email protected]>
Merge partly of open-telemetry#1844

Signed-off-by: owent <[email protected]>

Update gtest to 1.12.1 by default

Signed-off-by: owent <[email protected]>
Fix compatibility when using clang and libc++

Signed-off-by: owent <[email protected]>

No need build local gtest when using legacy toolchain.

Signed-off-by: owent <[email protected]>

Fix env in sudo

Signed-off-by: owent <[email protected]>

Add hint to print current C++ standard and stdlib for cmake.

Signed-off-by: owent <[email protected]>

Print output of C++ environment detection

Signed-off-by: owent <[email protected]>

Allow to set cxx flags by environments in CI jobs.

Signed-off-by: owent <[email protected]>

fix MAINTAINER_MODE:
- bump gcc to version 12
- bump clang to version 14
- expand coverage on W3C tests

Upgrade gootletest 1.12.1

Fixed new warnings.

Fixed format, added changelog.

Using fallback version of span when `std::span` do not support `range`.

Signed-off-by: owent <[email protected]>
Signed-off-by: owent <[email protected]>
@lalitb lalitb merged commit dfff693 into open-telemetry:main Dec 13, 2022
@owent owent deleted the fix_1851 branch January 13, 2023 07:07
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.

Can not compile when using clang with cmake and -DWITH_STL=ON
4 participants