Change C++ standard to C++20

facebookincubator · Sep 11, 2024 · e05dd84 · e05dd84
1 parent cc10dc2
commit e05dd84
Show file tree

Hide file tree

Showing 13 changed files with 39 additions and 15 deletions.
diff --git 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(

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/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
@@ -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
@@ -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/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
@@ -3952,7 +3952,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,12 @@ 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> tyep 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__) && (__apple_build_version__ < 15000309)
 template <class CharT, class Traits, class Duration>
 inline
 typename std::enable_if
@@ -3983,6 +3989,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__) && (__apple_build_version__ < 15000309)
 
 template <class CharT, class Traits>
 inline

diff --git a/velox/type/Subfield.h b/velox/type/Subfield.h
@@ -163,7 +163,7 @@ class Subfield {
     }
 
     std::string toString() const override {
-      return "[" + std::to_string(index_) + "]";
+      return std::string("[").append(std::to_string(index_)).append("]");
     }
 
     bool isSubscript() const override {

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);