From fe535930d3183b46d7e088683cbe8c49285715f8 Mon Sep 17 00:00:00 2001 From: Tony Liao Date: Fri, 18 Oct 2024 14:59:56 -0700 Subject: [PATCH] Bump minimum C++ version to C++17 after branch cut for v29. Branch cut for v29 is done on 2024-09-30: https://github.com/protocolbuffers/protobuf/releases/tag/v29.0-rc1 The next version v30 will be a breaking release. The release date is scheduled after the EOL of C++14 support on 2024-12-15 for Google open source projects generally: https://github.com/google/oss-policies-info/blob/main/foundational-cxx-support-matrix.md This commit allows us to start taking advantage of C++17 features now. Some issues I ran into while upgrading: Two GCC 9.5 bugs related to -Wunused-but-set-parameter: - https://godbolt.org/z/qo51cKe7b - https://godbolt.org/z/65qW3vGhP Another GCC warning related to -Wself-assign in a template. There is a custom ASAN check that is not yet open sourced. I'll see if I can open source them in a subsequent commit. #test-continuous PiperOrigin-RevId: 687435042 --- .github/workflows/test_cpp.yml | 35 ++++++++++--------- ci/Linux.bazelrc | 3 +- ci/Windows.bazelrc | 4 ++- ci/macOS.bazelrc | 3 +- examples/.bazelrc | 6 ++-- src/google/protobuf/arena_unittest.cc | 6 +--- src/google/protobuf/port_def.inc | 8 ++--- src/google/protobuf/proto3_arena_unittest.cc | 6 +--- .../protobuf/reflection_visit_field_info.h | 7 ++++ .../protobuf/reflection_visit_fields_test.cc | 14 +++++++- 10 files changed, 54 insertions(+), 38 deletions(-) diff --git a/.github/workflows/test_cpp.yml b/.github/workflows/test_cpp.yml index d41062023493..95b1fe909056 100644 --- a/.github/workflows/test_cpp.yml +++ b/.github/workflows/test_cpp.yml @@ -130,7 +130,7 @@ jobs: -c "set -ex; sccache -z; cmake . -DWITH_PROTOC=/workspace/${{ steps.cross-compile.outputs.protoc }} - -Dprotobuf_BUILD_LIBUPB=OFF -Dprotobuf_BUILD_CONFORMANCE=ON -DCMAKE_CXX_STANDARD=14 + -Dprotobuf_BUILD_LIBUPB=OFF -Dprotobuf_BUILD_CONFORMANCE=ON -DCMAKE_CXX_STANDARD=17 -Dprotobuf_WITH_ZLIB=OFF ${{ env.SCCACHE_CMAKE_FLAGS }}; cmake --build . --parallel 20; ctest --parallel 20; @@ -141,15 +141,13 @@ jobs: fail-fast: false # Don't cancel all jobs if one fails. matrix: include: - - flags: -Dprotobuf_BUILD_EXAMPLES=ON -DCMAKE_CXX_STANDARD=14 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF + - flags: -Dprotobuf_BUILD_EXAMPLES=ON -DCMAKE_CXX_STANDARD=17 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF - name: Ninja - flags: -G Ninja -DCMAKE_CXX_STANDARD=14 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF + flags: -G Ninja -DCMAKE_CXX_STANDARD=17 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF continuous-only: true - name: Shared - flags: -Dprotobuf_BUILD_SHARED_LIBS=ON -Dprotobuf_BUILD_EXAMPLES=ON -DCMAKE_CXX_STANDARD=14 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF + flags: -Dprotobuf_BUILD_SHARED_LIBS=ON -Dprotobuf_BUILD_EXAMPLES=ON -DCMAKE_CXX_STANDARD=17 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF continuous-only: true - - name: C++17 - flags: -DCMAKE_CXX_STANDARD=17 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF - name: C++20 flags: -DCMAKE_CXX_STANDARD=20 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF - name: Fetch @@ -217,7 +215,7 @@ jobs: image: us-docker.pkg.dev/protobuf-build/containers/test/linux/cmake:3.16.9-f39fc8b4e244fe5cd4c7138d0b6959a52b46ca48 credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} command: >- - /install.sh -DCMAKE_CXX_STANDARD=14 ${{ env.SCCACHE_CMAKE_FLAGS }} + /install.sh -DCMAKE_CXX_STANDARD=17 ${{ env.SCCACHE_CMAKE_FLAGS }} ${{ matrix.flags }} -Dprotobuf_BUILD_SHARED_LIBS=ON \&\& /test.sh @@ -225,7 +223,7 @@ jobs: -Dprotobuf_REMOVE_INSTALLED_HEADERS=ON -Dprotobuf_BUILD_PROTOBUF_BINARIES=OFF -Dprotobuf_BUILD_CONFORMANCE=ON - -DCMAKE_CXX_STANDARD=14 + -DCMAKE_CXX_STANDARD=17 ${{ matrix.flags }} # This test should always be skipped on presubmit @@ -253,12 +251,12 @@ jobs: image: us-docker.pkg.dev/protobuf-build/containers/test/linux/cmake:3.16.9-f39fc8b4e244fe5cd4c7138d0b6959a52b46ca48 credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} command: >- - /install.sh -DCMAKE_CXX_STANDARD=14 ${{ env.SCCACHE_CMAKE_FLAGS }} + /install.sh -DCMAKE_CXX_STANDARD=17 ${{ env.SCCACHE_CMAKE_FLAGS }} -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF -Dprotobuf_BUILD_EXAMPLES=OFF \&\& mkdir examples/build \&\& cd examples/build \&\& - cmake .. -DCMAKE_CXX_STANDARD=14 \&\& + cmake .. -DCMAKE_CXX_STANDARD=17 \&\& cmake --build . linux-cmake-gcc: @@ -266,8 +264,6 @@ jobs: fail-fast: false # Don't cancel all jobs if one fails. matrix: include: - - name: C++14 - flags: -DCMAKE_CXX_STANDARD=14 - name: C++17 flags: -DCMAKE_CXX_STANDARD=17 continuous-only: true @@ -331,7 +327,7 @@ jobs: /bin/bash -cex ' cd /workspace; sccache -z; - cmake . -DCMAKE_CXX_STANDARD=14 ${{ env.SCCACHE_CMAKE_FLAGS }}; + cmake . -DCMAKE_CXX_STANDARD=17 ${{ env.SCCACHE_CMAKE_FLAGS }}; cmake --build . --parallel 20; ctest --verbose --parallel 20; sccache -s' @@ -391,7 +387,6 @@ jobs: include: - name: MacOS CMake os: macos-13 - flags: -DCMAKE_CXX_STANDARD=14 cache-prefix: macos-cmake continuous-only: true - name: Windows CMake @@ -430,7 +425,9 @@ jobs: cache-prefix: windows-2022-cmake - name: Windows CMake Install os: windows-2022 - install-flags: -G Ninja -Dprotobuf_WITH_ZLIB=OFF -Dprotobuf_BUILD_CONFORMANCE=OFF -Dprotobuf_BUILD_TESTS=OFF + install-flags: >- + -G Ninja -Dprotobuf_WITH_ZLIB=OFF + -Dprotobuf_BUILD_CONFORMANCE=OFF -Dprotobuf_BUILD_TESTS=OFF flags: >- -G Ninja -Dprotobuf_WITH_ZLIB=OFF -Dprotobuf_BUILD_CONFORMANCE=OFF -Dprotobuf_REMOVE_INSTALLED_HEADERS=ON @@ -478,7 +475,9 @@ jobs: uses: protocolbuffers/protobuf-ci/bash@v3 with: credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} - command: cmake . ${{ matrix.install-flags }} ${{ env.SCCACHE_CMAKE_FLAGS }} -Dprotobuf_ALLOW_CCACHE=ON + command: >- + cmake . -DCMAKE_CXX_STANDARD=17 ${{ matrix.install-flags }} + ${{ env.SCCACHE_CMAKE_FLAGS }} -Dprotobuf_ALLOW_CCACHE=ON - name: Build for install if: ${{ matrix.install-flags && (!matrix.continuous-only || inputs.continuous-run) }} shell: bash @@ -501,7 +500,9 @@ jobs: uses: protocolbuffers/protobuf-ci/bash@v3 with: credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} - command: cmake . ${{ matrix.flags }} ${{ env.SCCACHE_CMAKE_FLAGS }} -Dprotobuf_ALLOW_CCACHE=ON + command: >- + cmake . -DCMAKE_CXX_STANDARD=17 ${{ matrix.flags }} + ${{ env.SCCACHE_CMAKE_FLAGS }} -Dprotobuf_ALLOW_CCACHE=ON - name: Build if: ${{ !matrix.continuous-only || inputs.continuous-run }} diff --git a/ci/Linux.bazelrc b/ci/Linux.bazelrc index 3cdf40d21844..fb91d049b8ad 100644 --- a/ci/Linux.bazelrc +++ b/ci/Linux.bazelrc @@ -1,5 +1,6 @@ import common.bazelrc -build --cxxopt=-std=c++14 --host_cxxopt=-std=c++14 +build --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 build --cxxopt="-Woverloaded-virtual" build --copt="-Werror" --copt="-Wno-sign-compare" --copt="-Wno-sign-conversion" --copt="-Wno-error=sign-conversion" --copt="-Wno-deprecated-declarations" + diff --git a/ci/Windows.bazelrc b/ci/Windows.bazelrc index dd3bb48b101a..97c3ef99e240 100644 --- a/ci/Windows.bazelrc +++ b/ci/Windows.bazelrc @@ -1,5 +1,7 @@ import common.bazelrc # Workaround for maximum path length issues +build --cxxopt=/std:c++17 --host_cxxopt=/std:c++17 startup --output_user_root=C:/tmp --windows_enable_symlinks -common --enable_runfiles \ No newline at end of file +common --enable_runfiles + diff --git a/ci/macOS.bazelrc b/ci/macOS.bazelrc index e91f62639a2d..847810e8a4a3 100644 --- a/ci/macOS.bazelrc +++ b/ci/macOS.bazelrc @@ -1,7 +1,8 @@ import common.bazelrc -build --cxxopt=-std=c++14 --host_cxxopt=-std=c++14 +build --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 build --cxxopt="-Woverloaded-virtual" build --copt="-Werror" --copt="-Wno-sign-compare" --copt="-Wno-sign-conversion" --copt="-Wno-error=sign-conversion" --copt="-Wno-deprecated-declarations" common --repo_env=BAZEL_NO_APPLE_CPP_TOOLCHAIN=1 common --xcode_version_config=@com_google_protobuf//.github:host_xcodes + diff --git a/examples/.bazelrc b/examples/.bazelrc index f37d75bc2112..7873d890ec7e 100644 --- a/examples/.bazelrc +++ b/examples/.bazelrc @@ -1,9 +1,9 @@ common --enable_platform_specific_config -build:linux --cxxopt=-std=c++14 --host_cxxopt=-std=c++14 -build:macos --cxxopt=-std=c++14 --host_cxxopt=-std=c++14 +build:linux --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 +build:macos --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 -common:windows --enable_runfiles +common:windows --cxxopt=/std:c++17 --host_cxxopt=/std:c++17 --enable_runfiles build --experimental_remote_cache_eviction_retries=5 build --remote_download_outputs=all diff --git a/src/google/protobuf/arena_unittest.cc b/src/google/protobuf/arena_unittest.cc index 7fe39ed8ae04..5923cce62b59 100644 --- a/src/google/protobuf/arena_unittest.cc +++ b/src/google/protobuf/arena_unittest.cc @@ -1541,11 +1541,7 @@ TEST(ArenaTest, ClearOneofMessageOnArena) { #ifndef PROTOBUF_ASAN EXPECT_NE(child->moo_int(), 100); -#else -#if GTEST_HAS_DEATH_TEST && defined(__cpp_if_constexpr) - EXPECT_DEATH(EXPECT_EQ(child->moo_int(), 0), "use-after-poison"); -#endif -#endif +#endif // !PROTOBUF_ASAN } TEST(ArenaTest, CopyValuesWithinOneof) { diff --git a/src/google/protobuf/port_def.inc b/src/google/protobuf/port_def.inc index b381f599df77..a6220e19c12e 100644 --- a/src/google/protobuf/port_def.inc +++ b/src/google/protobuf/port_def.inc @@ -116,7 +116,8 @@ static_assert(PROTOBUF_GNUC_MIN(7, 3), "Protobuf only supports GCC 7.3 and newer #elif defined(_MSVC_LANG) static_assert(PROTOBUF_MSC_VER_MIN(1910), "Protobuf only supports MSVC 2017 and newer."); #endif -static_assert(PROTOBUF_CPLUSPLUS_MIN(201402L), "Protobuf only supports C++14 and newer."); +static_assert(PROTOBUF_CPLUSPLUS_MIN(201703L), + "Protobuf only supports C++17 and newer."); // Check minimum Abseil version. #if defined(ABSL_LTS_RELEASE_VERSION) && defined(ABSL_LTS_RELEASE_PATCH_LEVEL) @@ -371,7 +372,7 @@ static_assert(PROTOBUF_ABSL_MIN(20230125, 3), #ifdef PROTOBUF_NODISCARD #error PROTOBUF_NODISCARD was previously defined #endif -#if ABSL_HAVE_CPP_ATTRIBUTE(nodiscard) && PROTOBUF_CPLUSPLUS_MIN(201703L) +#if ABSL_HAVE_CPP_ATTRIBUTE(nodiscard) #define PROTOBUF_NODISCARD [[nodiscard]] #elif ABSL_HAVE_ATTRIBUTE(warn_unused_result) || defined(__GNUC__) #define PROTOBUF_NODISCARD __attribute__((warn_unused_result)) @@ -615,8 +616,7 @@ static_assert(PROTOBUF_ABSL_MIN(20230125, 3), #ifdef PROTOBUF_UNUSED #error PROTOBUF_UNUSED was previously defined #endif -#if ABSL_HAVE_CPP_ATTRIBUTE(maybe_unused) || \ - (PROTOBUF_MSC_VER_MIN(1911) && PROTOBUF_CPLUSPLUS_MIN(201703L)) +#if ABSL_HAVE_CPP_ATTRIBUTE(maybe_unused) || (PROTOBUF_MSC_VER_MIN(1911)) #define PROTOBUF_UNUSED [[maybe_unused]] #elif ABSL_HAVE_ATTRIBUTE(unused) || defined(__GNUC__) #define PROTOBUF_UNUSED __attribute__((__unused__)) diff --git a/src/google/protobuf/proto3_arena_unittest.cc b/src/google/protobuf/proto3_arena_unittest.cc index 1c56ebee4014..f1ee7ed122c4 100644 --- a/src/google/protobuf/proto3_arena_unittest.cc +++ b/src/google/protobuf/proto3_arena_unittest.cc @@ -289,11 +289,7 @@ TEST(Proto3ArenaTest, CheckOneofMessageFieldIsCleared) { #ifndef PROTOBUF_ASAN EXPECT_EQ(child->bb(), 0); -#else -#if GTEST_HAS_DEATH_TEST && defined(__cpp_if_constexpr) - EXPECT_DEATH(EXPECT_EQ(child->bb(), 100), "use-after-poison"); -#endif -#endif +#endif // !PROTOBUF_ASAN } TEST(Proto3OptionalTest, OptionalFieldDescriptor) { diff --git a/src/google/protobuf/reflection_visit_field_info.h b/src/google/protobuf/reflection_visit_field_info.h index dee9c8526df6..d31d2962b19a 100644 --- a/src/google/protobuf/reflection_visit_field_info.h +++ b/src/google/protobuf/reflection_visit_field_info.h @@ -1137,6 +1137,13 @@ struct RepeatedGroupDynamicExtensionInfo // users from a similar dispatch without creating KeyInfo or ValueInfo per type. template inline size_t MapPrimitiveFieldByteSize(FieldDescriptor::Type type, T value) { + // There is a bug in GCC 9.5 where if-constexpr arguments are not understood + // if encased in a switch statement. A reproduction of the bug can be found + // at: https://godbolt.org/z/qo51cKe7b + // This is fixed in GCC 10.1+. + (void)type; // Suppress -Wunused-but-set-parameter + (void)value; // Suppress -Wunused-but-set-parameter + if constexpr (cpp_type == FieldDescriptor::CPPTYPE_INT32) { static_assert(std::is_same_v, "type mismatch"); switch (type) { diff --git a/src/google/protobuf/reflection_visit_fields_test.cc b/src/google/protobuf/reflection_visit_fields_test.cc index ac84893bd72d..75b7a05401e0 100644 --- a/src/google/protobuf/reflection_visit_fields_test.cc +++ b/src/google/protobuf/reflection_visit_fields_test.cc @@ -172,7 +172,7 @@ void MutateNothingByVisit(Message& message) { } } else { for (auto& it : info.Mutable()) { - it = it; + it = *⁢ // Avoid -Wself-assign. } } } else { @@ -207,6 +207,12 @@ TEST_P(VisitFieldsTest, MutateNothingByVisitIdempotent) { template inline size_t MapKeyByteSizeLong(FieldDescriptor::Type type, InfoT info) { + // There is a bug in GCC 9.5 where if-constexpr arguments are not understood + // if passed into a helper function. A reproduction of the bug can be found + // at: https://godbolt.org/z/65qW3vGhP + // This is fixed in GCC 10.1+. + (void)type; // Suppress -Wunused-but-set-parameter + if constexpr (info.cpp_type == FieldDescriptor::CPPTYPE_STRING) { return WireFormatLite::StringSize(info.Get()); } else { @@ -216,6 +222,12 @@ inline size_t MapKeyByteSizeLong(FieldDescriptor::Type type, InfoT info) { template inline size_t MapValueByteSizeLong(FieldDescriptor::Type type, InfoT info) { + // There is a bug in GCC 9.5 where if-constexpr arguments are not understood + // if passed into a helper function. A reproduction of the bug can be found + // at: https://godbolt.org/z/65qW3vGhP + // This is fixed in GCC 10.1+. + (void)type; // Suppress -Wunused-but-set-parameter + if constexpr (info.cpp_type == FieldDescriptor::CPPTYPE_STRING) { return WireFormatLite::StringSize(info.Get()); } else if constexpr (info.cpp_type == FieldDescriptor::CPPTYPE_MESSAGE) {