Skip to content

Commit

Permalink
feat(build): Switch to use C++20 standard
Browse files Browse the repository at this point in the history
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 ‘[=]’
- 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
  • Loading branch information
czentgr committed Nov 19, 2024
1 parent 3eb2fa4 commit 15205c4
Show file tree
Hide file tree
Showing 22 changed files with 103 additions and 27 deletions.
28 changes: 27 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,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(
Expand Down Expand Up @@ -350,6 +364,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
Expand All @@ -362,6 +383,11 @@ if(ENABLE_ALL_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})
Expand Down
26 changes: 26 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pyvelox/pyvelox.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
16 changes: 8 additions & 8 deletions scripts/setup-helper-functions.sh
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,15 @@ function get_cxx_flags {
case $CPU_ARCH in

"arm64")
echo -n "-mcpu=apple-m1+crc -std=c++17 -fvisibility=hidden"
echo -n "-mcpu=apple-m1+crc -fvisibility=hidden"
;;

"avx")
echo -n "-mavx2 -mfma -mavx -mf16c -mlzcnt -std=c++17 -mbmi2"
echo -n "-mavx2 -mfma -mavx -mf16c -mlzcnt -mbmi2"
;;

"sse")
echo -n "-msse4.2 -std=c++17"
echo -n "-msse4.2"
;;

"aarch64")
Expand All @@ -138,16 +138,16 @@ function get_cxx_flags {
ARM_CPU_PRODUCT=${hex_ARM_CPU_DETECT: -4:3}

if [ "$ARM_CPU_PRODUCT" = "$Neoverse_N1" ]; then
echo -n "-mcpu=neoverse-n1 -std=c++17"
echo -n "-mcpu=neoverse-n1"
elif [ "$ARM_CPU_PRODUCT" = "$Neoverse_N2" ]; then
echo -n "-mcpu=neoverse-n2 -std=c++17"
echo -n "-mcpu=neoverse-n2"
elif [ "$ARM_CPU_PRODUCT" = "$Neoverse_V1" ]; then
echo -n "-mcpu=neoverse-v1 -std=c++17"
echo -n "-mcpu=neoverse-v1"
else
echo -n "-march=armv8-a+crc+crypto -std=c++17"
echo -n "-march=armv8-a+crc+crypto"
fi
else
echo -n "-std=c++17"
echo -n ""
fi
;;
*)
Expand Down
7 changes: 6 additions & 1 deletion velox/common/base/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions velox/common/base/Semaphore.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion velox/common/base/tests/ExceptionTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
};

Expand Down
2 changes: 1 addition & 1 deletion velox/common/base/tests/Memcpy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
3 changes: 1 addition & 2 deletions velox/connectors/hive/iceberg/IcebergSplit.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
6 changes: 5 additions & 1 deletion velox/core/Expressions.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions velox/duckdb/conversion/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion velox/dwio/common/SelectiveColumnReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
2 changes: 2 additions & 0 deletions velox/dwio/parquet/tests/writer/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion velox/exec/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion velox/expression/tests/ExprEncodingsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,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);
Expand Down
2 changes: 1 addition & 1 deletion velox/expression/tests/ExprTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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}));
Expand Down
11 changes: 11 additions & 0 deletions velox/external/date/date.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions velox/functions/prestosql/aggregates/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion velox/tool/trace/OperatorReplayerBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,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);
};
Expand Down
2 changes: 1 addition & 1 deletion velox/vector/BaseVector.h
Original file line number Diff line number Diff line change
Expand Up @@ -1061,7 +1061,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));
}
};
2 changes: 1 addition & 1 deletion velox/vector/tests/utils/VectorMaker.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions velox/vector/tests/utils/VectorTestBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
Expand Down

0 comments on commit 15205c4

Please sign in to comment.