From b2b0f5ba716efa124c2aa5068a94674a8e5cbe11 Mon Sep 17 00:00:00 2001 From: Christian Zentgraf Date: Fri, 20 Sep 2024 10:19:49 -0700 Subject: [PATCH] Switch to use C++20 standard --- CMakeLists.txt | 23 ++++++++++++++++++- pyvelox/pyvelox.cpp | 2 +- velox/common/base/Semaphore.h | 4 ++-- velox/common/base/tests/Memcpy.cpp | 2 +- velox/connectors/hive/iceberg/IcebergSplit.h | 3 +-- velox/core/Expressions.h | 6 ++++- velox/dwio/common/SelectiveColumnReader.h | 2 +- velox/expression/tests/ExprEncodingsTest.cpp | 2 +- velox/expression/tests/ExprTest.cpp | 2 +- velox/external/date/date.h | 7 ++++++ .../prestosql/tests/utils/CMakeLists.txt | 2 +- velox/vector/tests/utils/VectorMaker.h | 2 +- velox/vector/tests/utils/VectorTestBase.h | 4 ++-- 13 files changed, 46 insertions(+), 15 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7f7cbc92f250f..3292259f52ea7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -282,7 +282,21 @@ endif() # Required so velox code can be used in a dynamic library set(CMAKE_POSITION_INDEPENDENT_CODE TRUE) -set(CMAKE_CXX_STANDARD 17) +# For C++20 support we need GNU GCC11 (or later versions) or Clang/AppleClang 15 +# (or later versions) to get support for the used features. +if(NOT + (("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" AND ${CMAKE_CXX_COMPILER_VERSION} + VERSION_GREATER_EQUAL 11) + OR (("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" + OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang") + AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 15))) + message( + FATAL_ERROR + "Unsupported compiler ${CMAKE_CXX_COMPILER_ID} with version ${CMAKE_CXX_COMPILER_VERSION} found." + ) +endif() + +set(CMAKE_CXX_STANDARD 20) set(CMAKE_CXX_STANDARD_REQUIRED True) execute_process( @@ -338,6 +352,13 @@ if("${ENABLE_ALL_WARNINGS}") -Wno-unused-result \ -Wno-format-overflow \ -Wno-strict-aliasing") + # Avoid compiler bug for GCC 12.2.1 + # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105329 + if (CMAKE_CXX_COMPILER_VERSION VERSION_EQUAL "12.2.1") + set(KNOWN_COMPILER_SPECIFIC_WARNINGS + "${KNOWN_COMPILER_SPECIFIC_WARNINGS} \ + -Wno-restrict") + endif() endif() set(KNOWN_WARNINGS diff --git a/pyvelox/pyvelox.cpp b/pyvelox/pyvelox.cpp index 8f39fb4b904ef..fbcc0c4510351 100644 --- a/pyvelox/pyvelox.cpp +++ b/pyvelox/pyvelox.cpp @@ -282,7 +282,7 @@ static void addExpressionBindings( std::vector inputs; names.reserve(name_input_map.size()); inputs.reserve(name_input_map.size()); - for (const std::pair& pair : + for (const std::pair& pair : name_input_map) { names.push_back(pair.first); inputs.push_back(pair.second); diff --git a/velox/common/base/Semaphore.h b/velox/common/base/Semaphore.h index 760a072627359..b52e574e458f2 100644 --- a/velox/common/base/Semaphore.h +++ b/velox/common/base/Semaphore.h @@ -63,8 +63,8 @@ class Semaphore { private: std::mutex mutex_; std::condition_variable cv_; - volatile int32_t count_; - volatile int32_t numWaiting_{0}; + std::atomic count_; + std::atomic numWaiting_{0}; }; } // namespace facebook::velox diff --git a/velox/common/base/tests/Memcpy.cpp b/velox/common/base/tests/Memcpy.cpp index 2ab51f87815e1..4d7aeca1c1905 100644 --- a/velox/common/base/tests/Memcpy.cpp +++ b/velox/common/base/tests/Memcpy.cpp @@ -74,7 +74,7 @@ int main(int argc, char** argv) { Semaphore sem(0); std::vector ops; ops.resize(FLAGS_threads); - volatile uint64_t totalSum = 0; + std::atomic totalSum = 0; uint64_t totalUsec = 0; for (auto repeat = 0; repeat < FLAGS_repeats; ++repeat) { // Read once through 'other' to clear cache effects. diff --git a/velox/connectors/hive/iceberg/IcebergSplit.h b/velox/connectors/hive/iceberg/IcebergSplit.h index 6c1f4cc6bc3f3..f05d28103b06d 100644 --- a/velox/connectors/hive/iceberg/IcebergSplit.h +++ b/velox/connectors/hive/iceberg/IcebergSplit.h @@ -18,11 +18,10 @@ #include #include "velox/connectors/hive/HiveConnectorSplit.h" +#include "velox/connectors/hive/iceberg/IcebergDeleteFile.h" namespace facebook::velox::connector::hive::iceberg { -struct IcebergDeleteFile; - struct HiveIcebergSplit : public connector::hive::HiveConnectorSplit { std::vector deleteFiles; diff --git a/velox/core/Expressions.h b/velox/core/Expressions.h index e9e9fa65f32fa..457c0b25d44b5 100644 --- a/velox/core/Expressions.h +++ b/velox/core/Expressions.h @@ -563,7 +563,11 @@ class LambdaTypedExpr : public ITypedExpr { if (*casted->type() != *this->type()) { return false; } - return *signature_ == *casted->signature_ && *body_ == *casted->body_; + return operator==(*casted); + } + + bool operator==(const LambdaTypedExpr& other) const { + return *signature_ == *other.signature_ && *body_ == *other.body_; } folly::dynamic serialize() const override; diff --git a/velox/dwio/common/SelectiveColumnReader.h b/velox/dwio/common/SelectiveColumnReader.h index 053db8884884f..5bbf17e1117b2 100644 --- a/velox/dwio/common/SelectiveColumnReader.h +++ b/velox/dwio/common/SelectiveColumnReader.h @@ -340,7 +340,7 @@ class SelectiveColumnReader { template inline void addValue(T value) { static_assert( - std::is_pod_v, + std::is_standard_layout_v, "General case of addValue is only for primitive types"); VELOX_DCHECK_NOT_NULL(rawValues_); VELOX_DCHECK_LE((numValues_ + 1) * sizeof(T), values_->capacity()); diff --git a/velox/expression/tests/ExprEncodingsTest.cpp b/velox/expression/tests/ExprEncodingsTest.cpp index 7eb5b2e91d0c1..4f741f4b5d519 100644 --- a/velox/expression/tests/ExprEncodingsTest.cpp +++ b/velox/expression/tests/ExprEncodingsTest.cpp @@ -458,7 +458,7 @@ class ExprEncodingsTest execCtx_->pool(), vector->type(), vector->size(), - std::make_unique([=](RowSet /*rows*/) { + std::make_unique([=, this](RowSet /*rows*/) { auto indices = makeIndices(vector->size(), [](auto row) { return row; }); return wrapInDictionary(indices, vector->size(), vector); diff --git a/velox/expression/tests/ExprTest.cpp b/velox/expression/tests/ExprTest.cpp index 100d0acb71b1b..78f413aace441 100644 --- a/velox/expression/tests/ExprTest.cpp +++ b/velox/expression/tests/ExprTest.cpp @@ -3958,7 +3958,7 @@ TEST_P(ParameterizedExprTest, cseOverLazyDictionary) { pool(), BIGINT(), 5, - std::make_unique([=](RowSet /*rows*/) { + std::make_unique([=, this](RowSet /*rows*/) { return wrapInDictionary( makeIndicesInReverse(5), makeFlatVector({8, 9, 10, 11, 12})); diff --git a/velox/external/date/date.h b/velox/external/date/date.h index ac6d636dd4283..ea29ea9d20646 100644 --- a/velox/external/date/date.h +++ b/velox/external/date/date.h @@ -3970,6 +3970,12 @@ make_time(const std::chrono::duration& d) return hh_mm_ss>(d); } +/// Building on macOS using Apple Clang 15.0.0.15000309 (Xcode_15.4) results in +/// ambiguous symbols (related to std::chrono::duration type parameter) because +/// the newer SDK includes a definitoon for this overloaded operator when C++20 standard is used. +/// AppleClang 15.0.0.15000309 (Xcode_15.4) or newer does not need the overloaded definition but +/// AppleClang 15.0.0.15000100 (Xcode_15.2), for example, does. +#if !defined(__APPLE__) || (defined(__APPLE__) && (__apple_build_version__ < 15000309)) template inline typename std::enable_if @@ -3983,6 +3989,7 @@ operator<<(std::basic_ostream& os, const sys_time& tp) auto const dp = date::floor(tp); return os << year_month_day(dp) << ' ' << make_time(tp-dp); } +#endif // !defined(__APPLE__) || (defined(__APPLE__) && (__apple_build_version__ < 15000309)) template inline diff --git a/velox/functions/prestosql/tests/utils/CMakeLists.txt b/velox/functions/prestosql/tests/utils/CMakeLists.txt index 9fad6d743a6d2..59567077c1798 100644 --- a/velox/functions/prestosql/tests/utils/CMakeLists.txt +++ b/velox/functions/prestosql/tests/utils/CMakeLists.txt @@ -17,6 +17,6 @@ add_library(velox_functions_test_lib FunctionBaseTest.cpp) target_link_libraries( velox_functions_test_lib velox_exec - velox_exec_test_lib + velox_functions_prestosql velox_vector_test_lib velox_parse_parser) diff --git a/velox/vector/tests/utils/VectorMaker.h b/velox/vector/tests/utils/VectorMaker.h index a047930dd035c..e8ee2de262ce9 100644 --- a/velox/vector/tests/utils/VectorMaker.h +++ b/velox/vector/tests/utils/VectorMaker.h @@ -145,7 +145,7 @@ class VectorMaker { pool_, CppToType::create(), size, - std::make_unique([=](RowSet rowSet) { + std::make_unique([=, this](RowSet rowSet) { // Populate requested rows with correct data and fill in gaps with // "garbage". SelectivityVector rows(rowSet.back() + 1, false); diff --git a/velox/vector/tests/utils/VectorTestBase.h b/velox/vector/tests/utils/VectorTestBase.h index b0a363527d9ca..50e23149acae5 100644 --- a/velox/vector/tests/utils/VectorTestBase.h +++ b/velox/vector/tests/utils/VectorTestBase.h @@ -781,7 +781,7 @@ class VectorTestBase { pool(), CppToType::create(), size, - std::make_unique([=](RowSet rows) { + std::make_unique([=, this](RowSet rows) { if (expectedSize.has_value()) { VELOX_CHECK_EQ(rows.size(), *expectedSize); } @@ -799,7 +799,7 @@ class VectorTestBase { pool(), vector->type(), vector->size(), - std::make_unique([=](RowSet /*rows*/) { + std::make_unique([=, this](RowSet /*rows*/) { auto indices = makeIndices(vector->size(), [](auto row) { return row; }); return wrapInDictionary(indices, vector->size(), vector);