Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(build): Switch to use C++20 standard #10866

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(build): Switch to use C++20 standard #10866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +314 to +315
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also check for specific the specific features we need (see docs) this would make this portable to other compilers. What are we using that's only in gcc 11? Afaik 10 should have full(?) C++20 support?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gcc11 and clang15 have full C++20 support. gcc10 has only partial C++20 support. And one other reason was to set a minimum of compilers as a base from which we can build and check for features?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there are a few features missing https://en.cppreference.com/w/cpp/compiler_support#cpp20

I am fine with setting a minimum version (should be documented in the readme if it's not) but this specific check will not trigger if someone uses the Intel compiler or something but I guess that would still throw an error once the compiler doesn't understand the C++20 flag so 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand now. I didn't think about the possibility of using other compilers (e.g. Intel) as we don't claim to support them.
In prestissimo we have a table with the compilers. I will add it to the Velox Readme too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a table in the readme. Please take a look.

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

Expand Down Expand Up @@ -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})
Expand Down
26 changes: 26 additions & 0 deletions README.md
View file
Open in desktop
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
View file
Open in desktop
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
7 changes: 6 additions & 1 deletion velox/common/base/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions velox/common/base/Semaphore.h
View file
Open in desktop
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_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just remove the volatile, these are already fully protected by the mutex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@majetideepak was concerned that without volatile some code might be optimized away that existed previously. There must have been some reason to use it before. So I changed it to use atomic to prevent any issue as this is the recommended change for situations like this.

Copy link
Contributor

@Yuhta Yuhta Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With modern compiler and CPU volatile is not needed. The only case I needed it in my career is in CUDA.

Copy link
Collaborator Author

@czentgr czentgr Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I defer to you on this. I have never used it either that I remember. I assume there was some reason the original author used it. Given we do have mutex protection single threaded access is guaranteed. I'm removing the std::atomic use here.

std::atomic<int32_t> numWaiting_{0};
};

} // namespace facebook::velox
2 changes: 1 addition & 1 deletion velox/common/base/tests/ExceptionTest.cpp
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions velox/duckdb/conversion/tests/CMakeLists.txt
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
11 changes: 11 additions & 0 deletions velox/external/date/date.h
View file
Open in desktop
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) || (__apple_build_version__ >= 16000026)))

template <class CharT, class Traits>
inline
Expand Down
1 change: 1 addition & 0 deletions velox/functions/prestosql/aggregates/CMakeLists.txt
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion velox/vector/BaseVector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
};
Loading