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

build: Compile with C++20 #411

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

mpwarres
Copy link
Contributor

std=c++20 is used by both v8 and Envoy. Switching to use it overall will reduce build surprises when importing proxy-wasm-cpp-host into Envoy.

Signed-off-by: Michael Warres <[email protected]>
@mpwarres
Copy link
Contributor Author

Windows and MacOS presubmits fail with:

In file included from external/com_google_googletest/googlemock/include/gmock/gmock.h:59:
external/com_google_googletest/googlemock/include/gmock/gmock-actions.h:819:36: error: no type named 'result_of' in namespace 'std'
  using ReturnType = typename std::result_of<MethodPtr(Class*)>::type;
                     ~~~~~~~~~~~~~~^~~~~~~~~
external/com_google_googletest/googlemock/include/gmock/gmock-actions.h:819:45: error: expected ';' after alias declaration
  using ReturnType = typename std::result_of<MethodPtr(Class*)>::type;

This should be fixable by upgrading to a more recent version of googletest: google/googletest#2914

Signed-off-by: Michael Warres <[email protected]>
@mpwarres
Copy link
Contributor Author

mpwarres commented Aug 18, 2024

Repro of g++-specific compile error with variadic templates, which compiles successfully with -std=c++17 but not with -std=c++20: https://godbolt.org/z/TT9a9YPf7

@mpwarres
Copy link
Contributor Author

Repro of g++-specific compile error with variadic templates, which compiles successfully with -std=c++17 but not with -std=c++20: https://godbolt.org/z/TT9a9YPf7

Changing this constructor declaration from:

Counter<Tags...>(std::string_view name, MetricTagDescriptor<Tags>... descriptors)

to:

Counter(std::string_view name, MetricTagDescriptor<Tags>... descriptors)

appears to appease gcc-12, and works with clang as well. Hooray for godbolt.

@mpwarres
Copy link
Contributor Author

clang-tidy failure may be llvm/llvm-project#56709

@mpwarres
Copy link
Contributor Author

Repro of g++-specific compile error with variadic templates, which compiles successfully with -std=c++17 but not with -std=c++20: https://godbolt.org/z/TT9a9YPf7

Changing this constructor declaration [...]
appears to appease gcc-12, and works with clang as well. Hooray for godbolt.

Looks like the issue was this. I'll put it in a PR for proxy-wasm-cpp-sdk tomorrow.

@kyessenov
Copy link
Collaborator

Repro of g++-specific compile error with variadic templates, which compiles successfully with -std=c++17 but not with -std=c++20: https://godbolt.org/z/TT9a9YPf7

Changing this constructor declaration from:

Counter<Tags...>(std::string_view name, MetricTagDescriptor<Tags>... descriptors)

to:

Counter(std::string_view name, MetricTagDescriptor<Tags>... descriptors)

appears to appease gcc-12, and works with clang as well. Hooray for godbolt.

I distinctly remember we had to appeal this way in envoy - there might be a patch there already.

@mpwarres
Copy link
Contributor Author

I distinctly remember we had to appeal this way in envoy - there might be a patch there already.

Indeed! https://github.com/envoyproxy/envoy/blob/6648db236f64218611f36e91285716c8bec67abb/bazel/proxy_wasm_cpp_sdk.patch

@mpwarres
Copy link
Contributor Author

The only remaining CI failure is due to clang-tidy itself crashing, which I hope is transient. So I think this is ready for review, @martijneken and/or @PiotrSikora. Thanks!

This isn't strictly necessary in this PR for CI to pass, however I encountered
a need for it in child PR proxy-wasm#409, and seemed more appropriate to add here.

Signed-off-by: Michael Warres <[email protected]>
@@ -0,0 +1,33 @@
# Address g++ -std=c++20 variadic template compilation issue:
Copy link
Member

@PiotrSikora PiotrSikora Aug 20, 2024

Choose a reason for hiding this comment

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

This should be fixed directly in the C++ SDK (which we also control) and bumped here instead.

mpwarres added a commit to mpwarres/proxy-wasm-cpp-sdk that referenced this pull request Oct 11, 2024
This is to appease stricter checks with g++ -std=c++20, details in
[this](proxy-wasm/proxy-wasm-cpp-host#411 (comment))
and subsequent comments.

Signed-off-by: Michael Warres <[email protected]>
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