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

c++20 by default #32585

Merged
merged 21 commits into from
Mar 4, 2024
Merged

c++20 by default #32585

merged 21 commits into from
Mar 4, 2024

Conversation

kyessenov
Copy link
Contributor

@kyessenov kyessenov commented Feb 26, 2024

Change-Id: Ia1908ecdcbf514638bb62807863420cdebacb9df

Commit Message: Update all builds except mobile to use C++20 language reference.
Additional Description:
Risk Level: medium
Testing: done
Docs Changes: none
Release Notes: none

Change-Id: Ia1908ecdcbf514638bb62807863420cdebacb9df
Signed-off-by: Kuat Yessenov <kuat@google.com>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Feb 26, 2024
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #32585 was opened by kyessenov.

see: more, trace.

Change-Id: I8aaf137601c92e7a9a87ded43693b86f0945d9c7
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: I85da5b74dde0c64e9f467f9600e8020971e6e684
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: I1cc53913d7b66293bb6ff7c5d8c837917fb79b4e
Signed-off-by: Kuat Yessenov <kuat@google.com>
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

LGTM, assuming we can sort out CI!

Change-Id: I88d490addcc68ae8508b29fec2505f906aedb1fd
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: I4b919b0aa8774458e4efe39d91603d28e6a649bc
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: I98a310a30a7664a2512be110ead21b245ae7d126
Signed-off-by: Kuat Yessenov <kuat@google.com>
This reverts commit badd720.

Change-Id: I1d5eb6c3427e467367fdde378abb28dada2d7991
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: I181a9d3aeeb3c89137048c8a9d9ff74d8c61ad25
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: Icd5325d65a1de6e8f67a546b1afda697a85e9501
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: I24f648d496808e65926389ff6f1c3b3735adef41
Change-Id: Ia6a40ca21602c2cd726ea43c0a93ee3f99811647
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov kyessenov requested a review from lizan as a code owner February 27, 2024 21:43
@RyanTheOptimist
Copy link
Contributor

/wait on CI

Change-Id: Iff80bafd95591618c586e8c61b0084af4bd4557a
@kyessenov
Copy link
Contributor Author

/retest

@kyessenov
Copy link
Contributor Author

So close! gcc and android NDK are the last two standing.

Change-Id: Iac3690e3eee10e16f7679f1524e2f3c1835bbf96
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: I5ae73de42a93b5b17b948d2d17a10f8b075faca9
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Contributor Author

GCC done. Only NDK remains.

Change-Id: Id663fb0a88a9eb2968a86539054f7ef87aa518e6
Change-Id: Ie7538fb6c6a62b8c221cad51e4a4986753119a17
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: I162b3c89507e4ac7340836a3f89c1cb55ae0d850
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: I09432434403c924d433051f1ace208c533a28993
Signed-off-by: Kuat Yessenov <kuat@google.com>
Change-Id: Ic33826545f4528f4f26caf9d9f06e93079cf2a9f
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Contributor Author

https://github.com/envoyproxy/envoy/actions/runs/8103764117 confirms c++17 restriction works on Android build and rejects C++20 constructs.

Change-Id: I12c9ab575960ac69bf1e65baf5374932cacbfbbf
Signed-off-by: Kuat Yessenov <kuat@google.com>
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

LGTM, modulo a couple of nits

- static_assert(std::is_pointer_interconvertible_base_of_v<Data, F>,
- "F must be pointer interconvertible to Data");
+// static_assert(std::is_pointer_interconvertible_base_of_v<Data, F>,
+// "F must be pointer interconvertible to Data");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the "//"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan to update cel-cpp immediately and get rid of this patch or fix it upstream. I can't tell if this is a real issue or not with GCC.


- Counter<Tags...>(std::string_view name, MetricTagDescriptor<Tags>... descriptors)
- : Counter<Tags...>(std::string(name), std::vector<MetricTag>({toMetricTag(descriptors)...})) {
+ template <typename... T>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is proxy_wasm_api incompatible with C++20?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is just some helper AFAICT which is only used in one place in a test.

@@ -271,7 +271,14 @@ void Utility::extractCommonAccessLogProperties(
}

if (stream_info.upstreamInfo().has_value()) {
#if defined(__GNUC__) && !defined(__clang__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdangling-reference"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we have quite a few of these. Is -Wdangling-reference finding false positives? Should we consider just disabling this warning in our build globally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, gcc implementation is buggy, you can find reports in other projects. The pattern here is unusual but the same, so maybe better keep it since we know when the false positive happens.

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Mar 1, 2024
@kyessenov kyessenov merged commit 11d79fe into envoyproxy:main Mar 4, 2024
54 checks passed
@kyessenov kyessenov deleted the cpp20 branch March 4, 2024 17:31
mattjo added a commit to mattjo/envoy that referenced this pull request Mar 5, 2024
* main: (151 commits)
  http3: fixing an upstream threading issue and bumping http3 upstream code back to alpha (envoyproxy#32640)
  rlqs: reset quota usage (envoyproxy#32569)
  proxy status: add more mapping to proxystatus (envoyproxy#32606)
  rlqs: add logging around token bucket (envoyproxy#32612)
  Logging: ENVOY_BUG include filter name. (envoyproxy#32663)
  mobile: Reenable the FilterIntegrationTest.AltSvcCachedH2Slow test (envoyproxy#32675)
  c++20 by default (envoyproxy#32585)
  Docs: Add diagram for histogram stat sink. (envoyproxy#32665)
  Fix null node for list of struct in payload_to_metadaata filter (envoyproxy#32309)
  metrics_service: populate histogram summary sample sum (envoyproxy#32666)
  build(deps): bump postgres from `0e564da` to `f58300a` in /examples/shared/postgres (envoyproxy#32632)
  build(deps): bump the examples-ext-authz group in /examples/ext_authz with 1 update (envoyproxy#32654)
  build(deps): bump distroless/base-nossl-debian12 from `28dc895` to `0e777c6` in /ci (envoyproxy#32652)
  Update QUICHE from 02047e04d to 3373df94b (envoyproxy#32650)
  ci/logging: Add failure detection (envoyproxy#32662)
  Change udpa renaming workaround to not compile the same archive twice (envoyproxy#32647)
  sockets: flipping graceful client socket creation failure (envoyproxy#32602)
  TcpAsyncClient: enhance reconnect robustness (envoyproxy#32578)
  owners: adding Fredy as an Enovy Mobile maintainer (envoyproxy#32638)
  ci: Add scheduled garbage collection (and fix retest) (envoyproxy#32639)
  ...
mum4k pushed a commit to envoyproxy/nighthawk that referenced this pull request Mar 5, 2024
- Update the ENVOY_COMMIT and ENVOY_SHA in bazel/repositories.bzl to the latest Envoy's commit.
- Update to .bazelrc and tools/gen_compilation_database.py to envoyproxy/envoy#32585
- Update source/client/process_impl.cc to accommodate refactor in envoyproxy/envoy#32587
- Update tools/code_format/config.yaml for changes in envoyproxy/envoy#32625 and envoyproxy/envoy#32252
- Update python dependencies
- Ensure ostream_formatter definitions are before usage to prevent "explicit specialization of X after instantiation" errors

Signed-off-by: Tom Zhang <4367421+tomjzzhang@users.noreply.github.com>
abeyad added a commit to abeyad/envoy that referenced this pull request Mar 21, 2024
It was defaulting to C++20 due to
envoyproxy#32585, but for mobile, we are
still on C++17. This change enables local builds to build with C++17
without encountering toolchain errors.

Signed-off-by: Ali Beyad <abeyad@google.com>
abeyad added a commit that referenced this pull request Mar 21, 2024
It was defaulting to C++20 due to
#32585, but for mobile, we are
still on C++17. This change enables local builds to build with C++17
without encountering toolchain errors.

Signed-off-by: Ali Beyad <abeyad@google.com>
leonm1 added a commit to proxy-wasm/proxy-wasm-cpp-sdk that referenced this pull request Jan 7, 2025
Added in envoyproxy/envoy#32585.

Required for proxy-wasm/proxy-wasm-cpp-host#411

Signed-off-by: Matt Leon <mattleon@google.com>
leonm1 added a commit to leonm1/proxy-wasm-cpp-sdk that referenced this pull request Jan 7, 2025
Added in envoyproxy/envoy#32585.

Required for proxy-wasm/proxy-wasm-cpp-host#411

Signed-off-by: Matt Leon <mattleon@google.com>
leonm1 added a commit to leonm1/proxy-wasm-cpp-sdk that referenced this pull request Jan 7, 2025
Added in envoyproxy/envoy#32585.

Required for proxy-wasm/proxy-wasm-cpp-host#411

Signed-off-by: Matt Leon <mattleon@google.com>
mpwarres pushed a commit to proxy-wasm/proxy-wasm-cpp-sdk that referenced this pull request Jan 7, 2025
Added in envoyproxy/envoy#32585.

Required for proxy-wasm/proxy-wasm-cpp-host#411

Signed-off-by: Matt Leon <mattleon@google.com>
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.

2 participants