feat(build): 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 - turn on warning suppression for deprecated usage of implicit capture of ‘this’ via ‘[=]’ - fix ambiguity when calling == operator for LambdaTypedExpr - rearrange some link libraries order to prevent undefined symbols - handle AppleClang versions overloading operator<< for sys_seconds input - handle AppleClang format_to function ambiguity between fmt and system headers - remove std=c++17 from the setup helper script which influences the build - disable folly coroutine headers because the library is not built to support it If the implicit capture of this is fixed it causes upstream problems by issuing warning c++20-extensions as upstream users are not yet on C++20. Thus, adding to suppress the warning -Wdeprecated-this-capture for now.
facebookincubator · Dec 10, 2024 · 33f0a8f · 33f0a8f
1 parent 929affe
commit 33f0a8f
Show file tree

Hide file tree

Showing 16 changed files with 89 additions and 13 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -311,7 +311,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)
 set(CMAKE_CXX_EXTENSIONS ON) # Big Int is an extension
 
@@ -368,18 +382,31 @@ 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
       "-Wno-unused \
        -Wno-unused-parameter \
        -Wno-sign-compare \
        -Wno-ignored-qualifiers \
+       -Wno-deprecated-this-capture \
        ${KNOWN_COMPILER_SPECIFIC_WARNINGS}")
 
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra ${KNOWN_WARNINGS}")
 endif()
 
+# Folly is not built with coroutine support (for now) so avoid using the
+# coroutine headers as they pull in symbols that are not found in the folly
+# libraries. These will be removed once we build folly with coroutine support.
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DFOLLY_CFG_NO_COROUTINES")
+
 message("FINAL CMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS}")
 
 if(${VELOX_ENABLE_GPU})

diff --git a/README.md b/README.md
@@ -112,6 +112,32 @@ Details on the dependencies and how Velox manages some of them for you
 Velox also provides the following scripts to help developers setup and install Velox
 dependencies for a given platform.
 
+### Supported OS and compiler matrix
+
+The minimum versions of supported compilers:
+
+| OS | Compiler | Version |
+|----|----------|---------|
+| Linux | gcc | 11 |
+| Linux | clang | 15 |
+| macOS | clang | 15 |
+
+The recommended OS versions and compilers:
+
+| OS | Compiler | Version |
+|----|----------|---------|
+| CentOS 9/RHEL 9 | gcc | 12 |
+| Ubuntu 22.04 | gcc | 11 |
+| macOS | clang | 16 |
+
+Alternative combinations:
+
+| OS | Compiler | Version |
+|----|----------|---------|
+| CentOS 9/RHEL 9 | gcc | 11 |
+| Ubuntu 20.04 | gcc | 11 |
+| Ubuntu 24.04 | clang | 15 |
+
 ### Setting up dependencies
 
 The following setup scripts use the `DEPENDENCY_DIR` environment variable to set the

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
@@ -43,7 +43,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/ExceptionTest.cpp b/velox/common/base/tests/ExceptionTest.cpp
@@ -41,7 +41,7 @@ struct fmt::formatter<Counter> {
   template <typename FormatContext>
   auto format(const Counter& c, FormatContext& ctx) const {
     auto x = c.counter++;
-    return format_to(ctx.out(), "{}", x);
+    return fmt::format_to(ctx.out(), "{}", x);
   }
 };
 

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
@@ -591,7 +591,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
@@ -335,7 +335,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
@@ -238,8 +238,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/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) || (__apple_build_version__ >= 16000026)))
 
 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/vector/BaseVector.h b/velox/vector/BaseVector.h
@@ -1068,7 +1068,7 @@ struct fmt::formatter<facebook::velox::VectorEncoding::Simple> {
   auto format(
       const facebook::velox::VectorEncoding::Simple& x,
       FormatContext& ctx) const {
-    return format_to(
+    return fmt::format_to(
         ctx.out(), "{}", facebook::velox::VectorEncoding::mapSimpleToName(x));
   }
 };