Switch to use C++20 standard

Fixes as a result: - fix warnings with deprecated usage of volatile variables - replace with std::atomic - replace usage of deprecated std::is_pod_v - fix deprecated usage of implicit capture of ‘this’ via ‘[=]’ - rearrange some link libraries order to prevent undefined symbols - handle AppleClang versions overloading operator<< for sys_seconds input
facebookincubator · Oct 16, 2024 · 7e33c29 · 7e33c29
1 parent 757a1e4
commit 7e33c29
Show file tree

Hide file tree

Showing 18 changed files with 72 additions and 17 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -299,7 +299,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(
@@ -355,6 +369,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
@@ -367,6 +388,16 @@ if("${ENABLE_ALL_WARNINGS}")
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra ${KNOWN_WARNINGS}")
 endif()
 
+# AppleClang16 issues warning that coroutines are not supported while Folly
+# headers attempt to use the std::experimental namespace that is not available,
+# thus triggering build errors: "/include/c++/v1/experimental/coroutine:64:5:
+# error: <experimental/coroutine> cannot be used with this compiler". Thus,
+# force Folly to not use the coroutines headers on macOS.
+if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang"
+   AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 16)
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DFOLLY_CFG_NO_COROUTINES")
+endif()
+
 message("FINAL CMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS}")
 
 if(${VELOX_ENABLE_GPU})

diff --git a/pyvelox/pyvelox.cpp b/pyvelox/pyvelox.cpp
@@ -282,7 +282,7 @@ static void addExpressionBindings(
             std::vector<VectorPtr> inputs;
             names.reserve(name_input_map.size());
             inputs.reserve(name_input_map.size());
-            for (const std::pair<std::string, VectorPtr>& pair :
+            for (const std::pair<const std::string, VectorPtr>& pair :
                  name_input_map) {
               names.push_back(pair.first);
               inputs.push_back(pair.second);

diff --git a/velox/common/base/CMakeLists.txt b/velox/common/base/CMakeLists.txt
@@ -42,7 +42,12 @@ velox_add_library(
 velox_link_libraries(
   velox_common_base
   PUBLIC velox_exception Folly::folly fmt::fmt xsimd
-  PRIVATE velox_common_compression velox_process velox_test_util glog::glog)
+  PRIVATE
+    velox_caching
+    velox_common_compression
+    velox_process
+    velox_test_util
+    glog::glog)
 
 if(${VELOX_BUILD_TESTING})
   add_subdirectory(tests)

diff --git 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<int32_t> count_;
+  std::atomic<int32_t> numWaiting_{0};
 };
 
 } // namespace facebook::velox
diff --git 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<CopyCallable> ops;
   ops.resize(FLAGS_threads);
-  volatile uint64_t totalSum = 0;
+  std::atomic<uint64_t> 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
@@ -18,11 +18,10 @@
 #include <string>
 
 #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<IcebergDeleteFile> deleteFiles;
 

diff --git a/velox/core/Expressions.h b/velox/core/Expressions.h
@@ -565,7 +565,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/duckdb/conversion/tests/CMakeLists.txt b/velox/duckdb/conversion/tests/CMakeLists.txt
@@ -20,6 +20,7 @@ add_test(velox_duckdb_conversion_test velox_duckdb_conversion_test)
 target_link_libraries(
   velox_duckdb_conversion_test
   velox_duckdb_parser
+  velox_common_base
   velox_parse_expression
   velox_functions_prestosql
   velox_functions_lib

diff --git a/velox/dwio/common/SelectiveColumnReader.h b/velox/dwio/common/SelectiveColumnReader.h
@@ -340,7 +340,7 @@ class SelectiveColumnReader {
   template <typename T>
   inline void addValue(T value) {
     static_assert(
-        std::is_pod_v<T>,
+        std::is_standard_layout_v<T>,
         "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/dwio/parquet/tests/writer/CMakeLists.txt b/velox/dwio/parquet/tests/writer/CMakeLists.txt
@@ -24,6 +24,7 @@ target_link_libraries(
   velox_dwio_parquet_writer
   velox_dwio_common_test_utils
   velox_vector_fuzzer
+  velox_caching
   Boost::regex
   velox_link_libs
   Folly::folly
@@ -42,6 +43,7 @@ target_link_libraries(
   velox_parquet_writer_test
   velox_dwio_parquet_writer
   velox_dwio_common_test_utils
+  velox_caching
   velox_link_libs
   Boost::regex
   Folly::folly

diff --git a/velox/exec/tests/CMakeLists.txt b/velox/exec/tests/CMakeLists.txt
@@ -236,8 +236,8 @@ add_executable(velox_cache_fuzzer_test CacheFuzzerTest.cpp)
 
 target_link_libraries(
   velox_cache_fuzzer_test
-  velox_cache_fuzzer
   velox_fuzzer_util
+  velox_cache_fuzzer
   GTest::gtest
   GTest::gtest_main)
 

diff --git 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<SimpleVectorLoader>([=](RowSet /*rows*/) {
+        std::make_unique<SimpleVectorLoader>([=, 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
@@ -3958,7 +3958,7 @@ TEST_P(ParameterizedExprTest, cseOverLazyDictionary) {
           pool(),
           BIGINT(),
           5,
-          std::make_unique<SimpleVectorLoader>([=](RowSet /*rows*/) {
+          std::make_unique<SimpleVectorLoader>([=, this](RowSet /*rows*/) {
             return wrapInDictionary(
                 makeIndicesInReverse(5),
                 makeFlatVector<int64_t>({8, 9, 10, 11, 12}));

diff --git a/velox/external/date/date.h b/velox/external/date/date.h
@@ -3970,6 +3970,16 @@ make_time(const std::chrono::duration<Rep, Period>& d)
     return hh_mm_ss<std::chrono::duration<Rep, Period>>(d);
 }
 
+/// Building on macOS using Apple Clang 15.0.0.15000309 (Xcode_15.4) results in
+/// ambiguous symbols (related to std::chrono::duration<long long> type parameter) because
+/// the newer SDK includes a definiton 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.
+/// With the introduction of AppleClang 1600.0.26.3 the behavior reverts back to the 15.0.0.15000100
+/// or earlier behavior and the function requires the overload again.
+/// This check is for allowing multiple versions AppleClang to build Velox.
+#if !defined(__APPLE__) || \
+    (defined(__APPLE__) && ((__apple_build_version__ < 15000309) || (__apple_build_version__ >= 16000026)))
 template <class CharT, class Traits, class Duration>
 inline
 typename std::enable_if
@@ -3983,6 +3993,7 @@ operator<<(std::basic_ostream<CharT, Traits>& os, const sys_time<Duration>& tp)
     auto const dp = date::floor<days>(tp);
     return os << year_month_day(dp) << ' ' << make_time(tp-dp);
 }
+#endif // !defined(__APPLE__) || (defined(__APPLE__) && (__apple_build_version__ < 15000309))
 
 template <class CharT, class Traits>
 inline

diff --git a/velox/functions/prestosql/aggregates/CMakeLists.txt b/velox/functions/prestosql/aggregates/CMakeLists.txt
@@ -53,6 +53,7 @@ velox_link_libraries(
   velox_exec
   velox_expression
   velox_presto_serializer
+  velox_presto_types
   velox_functions_aggregates
   velox_functions_lib
   velox_functions_util

diff --git a/velox/tool/trace/OperatorReplayerBase.cpp b/velox/tool/trace/OperatorReplayerBase.cpp
@@ -76,7 +76,8 @@ core::PlanNodePtr OperatorReplayerBase::createPlan() const {
 
 std::function<core::PlanNodePtr(std::string, core::PlanNodePtr)>
 OperatorReplayerBase::replayNodeFactory(const core::PlanNode* node) const {
-  return [=](const core::PlanNodeId& nodeId,
+  return [=, this](
+             const core::PlanNodeId& nodeId,
              const core::PlanNodePtr& source) -> core::PlanNodePtr {
     return createPlanNode(node, nodeId, source);
   };

diff --git a/velox/vector/tests/utils/VectorMaker.h b/velox/vector/tests/utils/VectorMaker.h
@@ -145,7 +145,7 @@ class VectorMaker {
         pool_,
         CppToType<T>::create(),
         size,
-        std::make_unique<SimpleVectorLoader>([=](RowSet rowSet) {
+        std::make_unique<SimpleVectorLoader>([=, 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
@@ -781,7 +781,7 @@ class VectorTestBase {
         pool(),
         CppToType<T>::create(),
         size,
-        std::make_unique<SimpleVectorLoader>([=](RowSet rows) {
+        std::make_unique<SimpleVectorLoader>([=, 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<SimpleVectorLoader>([=](RowSet /*rows*/) {
+        std::make_unique<SimpleVectorLoader>([=, this](RowSet /*rows*/) {
           auto indices =
               makeIndices(vector->size(), [](auto row) { return row; });
           return wrapInDictionary(indices, vector->size(), vector);