build(cmake): Lower minimum Clang version for Velox compatibility: #72
Conversation
WalkthroughCentralizes compiler version checks by introducing cmake/Toolchains/utils.cmake with validate_compiler_versions() and invoking it early in CMakeLists.txt; removes previous per-compiler checks. Deletes TraceableException.hpp and its test, updating error_handling CMake to drop the header. Replaces std::ranges usage with iterator-based algorithms in Array and tests. Adjusts CI to stop installing llvm@16; minor comment tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CMake
participant ToolchainUtils as Toolchains/utils.cmake
User->>CMake: Configure project
CMake->>ToolchainUtils: include(utils) and call validate_compiler_versions()
ToolchainUtils-->>CMake: Validate CMAKE_CXX_COMPILER_ID/version
alt Unsupported or too low
ToolchainUtils-->>User: FATAL_ERROR
else Supported
CMake-->>User: Continue configuration
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
cmake/ystdlib-helpers.cmake (1)
34-45: Action Required: C++20-only features detected against the new C++17 baselineA project-wide scan shows real C++20 constructs still in use, so simply lowering to C++17 will break builds. You’ll need to either refactor these to C++17-compatible code or restore C++20 where needed.
Detected C++20-only usage:
- src/ystdlib/error_handling/utils.hpp
• std::source_location- src/ystdlib/error_handling/ErrorCode.hpp
• concept + requires- src/ystdlib/containers/Array.hpp
• requires-clause, std::ranges::copy- examples/src/linking-tests.cpp
• std::ranges::equal- Multiple test files (e.g. containers/test/test_Array.cpp, error_handling/test)
• std::ranges algorithms- examples/CMakeLists.txt:
• target_compile_features(linking-tests PRIVATE cxx_std_20)Next steps:
- Refactor or remove C++20-only APIs (concepts, ranges, source_location) to target C++17, or
- Revert the C++17 downgrade for targets that require C++20 (adjust add_cpp_library or per-target CMake settings)
- Clean up any remaining
cxx_std_20flags in CMakeLists.txt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
CMakeLists.txt(2 hunks)cmake/ystdlib-helpers.cmake(5 hunks)src/ystdlib/error_handling/CMakeLists.txt(0 hunks)src/ystdlib/error_handling/TraceableException.hpp(0 hunks)src/ystdlib/error_handling/test/test_TraceableException.cpp(0 hunks)
💤 Files with no reviewable changes (3)
- src/ystdlib/error_handling/CMakeLists.txt
- src/ystdlib/error_handling/test/test_TraceableException.cpp
- src/ystdlib/error_handling/TraceableException.hpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
📚 Learning: 2025-06-06T15:01:53.987Z
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
Applied to files:
CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-ystdlib-cpp (macos-15, release)
- GitHub Check: build-ystdlib-cpp (macos-14, release)
- GitHub Check: build-ystdlib-cpp (macos-15, debug)
- GitHub Check: build-ystdlib-cpp (macos-14, debug)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (4)
cmake/ystdlib-helpers.cmake (1)
90-90: Correct propagation of C++17 via target_compile_featuresUsing cxx_std_17 on INTERFACE (header-only), PUBLIC (non-interface), and PRIVATE (tests) is the right approach to enforce the minimum standard on consumers and tests.
Also applies to: 101-101, 156-156
CMakeLists.txt (3)
13-19: C++17 shift and compiler minima update LGTMAligns with Velox constraints; GNU stays at 11, AppleClang/Clang lowered to 15. Comment link to C++17 support is helpful.
78-78: Unified test target now enforces C++17 — goodTest aggregate target correctly switched to cxx_std_17.
16-19: CI Configurations Already Support Clang ≥15The GitHub workflows install and run:
- On macos-14:
brew install llvm@16and pointCC/CXXto clang-16 (≥15).- On macos-15: the default Xcode toolchain includes AppleClang 15.x.
No further CI/toolchain changes are needed to satisfy the new minimum of Clang/AppleClang 15+. 🎉
Bill-hbrhbr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
CMakeLists.txt (1)
63-63: Inconsistent C++ standard requirementThe CMakeLists.txt still uses
cxx_std_20on line 63, which contradicts the PR objective of downgrading to C++17. This inconsistency could cause build failures or unexpected behaviour.- target_compile_features(${UNIFIED_UNIT_TEST_TARGET} PRIVATE cxx_std_20) + target_compile_features(${UNIFIED_UNIT_TEST_TARGET} PRIVATE cxx_std_17)
♻️ Duplicate comments (1)
cmake/ystdlib-helpers.cmake (1)
117-117: Inconsistent C++ standard comment - should reference C++17The comment mentions "C++20" but according to the PR objectives, the C++ standard requirement is being downgraded to C++17. This creates confusion as the comment doesn't match the intended standard change.
-# Adds a C++20 test executable named `unit-test-NAME` that will be built with `SOURCES` and linked +# Adds a C++17 test executable named `unit-test-NAME` that will be built with `SOURCES` and linked
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
CMakeLists.txt(1 hunks)cmake/Toolchains/llvm-clang-15-toolchain.cmake(1 hunks)cmake/Toolchains/utils.cmake(1 hunks)cmake/ystdlib-helpers.cmake(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
📚 Learning: 2025-06-06T15:01:53.987Z
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
Applied to files:
cmake/ystdlib-helpers.cmakeCMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04, release)
- GitHub Check: build-ystdlib-cpp (macos-15, release)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04, release)
- GitHub Check: build-ystdlib-cpp (macos-15, debug)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04, debug)
- GitHub Check: build-ystdlib-cpp (macos-14, debug)
- GitHub Check: build-ystdlib-cpp (macos-14, release)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04, debug)
🔇 Additional comments (5)
cmake/Toolchains/llvm-clang-15-toolchain.cmake (1)
13-15: Fix line continuation formattingThe line continuation backslash should be at the end of line 13, not at the beginning of line 14. This violates proper CMake formatting conventions.
- "Failed to locate LLVM v15 using Homebrew. Please ensure llvm@15 is installed: 'brew \ - install llvm@15'" + "Failed to locate LLVM v15 using Homebrew. Please ensure llvm@15 is installed: 'brew \ +install llvm@15'"⛔ Skipped due to learnings
Learnt from: Bill-hbrhbr PR: y-scope/ystdlib-cpp#32 File: CMakeLists.txt:0-0 Timestamp: 2025-02-28T05:04:33.616Z Learning: Bill-hbrhbr maintains a maximum line length of 100 characters in CMake files and uses backslash line continuations to maintain this limit.CMakeLists.txt (2)
3-6: LGTM - Proper pre-project toolchain setupThe placement of toolchain setup before the
project()call is correct and follows CMake best practices. The conditional comment about top-level projects is also helpful.
10-10: LGTM - Post-project compiler validationCompiler validation after the
project()call is appropriately placed since compiler information is only available after project initialization.cmake/Toolchains/utils.cmake (2)
1-2: Update comment to reflect C++17 requirementThe comment references C++20 features, but according to the PR objectives, the project is downgrading to C++17. This creates confusion about the actual requirements.
-# This file contains utility functions for setting up toolchains and validating toolchain versions -# to ensure compatibility with the C++20 features required by the project. +# This file contains utility functions for setting up toolchains and validating toolchain versions +# to ensure compatibility with the C++17 features required by the project.
24-28: Update function comment to reflect C++17 requirementThe function comment still references C++20 features, which is inconsistent with the PR objective of downgrading to C++17.
-# Checks if the compiler ID and version meet the minimum requirements to support C++20 features +# Checks if the compiler ID and version meet the minimum requirements to support C++17 features # required by the project: # - AppleClang: version 15+ # - Clang: version 15+ # - GNU: version 11+
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
src/ystdlib/error_handling/test/types.cpp (1)
64-68: Avoid default lambda capture and pass the element by const referenceThis prevents accidental captures and avoids copying each element during the predicate call. No behaviour change.
- return std::any_of( - cFailureConditions.cbegin(), - cFailureConditions.cend(), - [&](auto failure_condition) -> bool { return condition == failure_condition; } - ); + return std::any_of( + cFailureConditions.cbegin(), + cFailureConditions.cend(), + [&condition](auto const& failure_condition) -> bool { + return condition == failure_condition; + } + );src/ystdlib/containers/Array.hpp (2)
108-112: Outdated TODO references removed TraceableException. Update the comment.The TODO suggests switching to TraceableException, which this PR removes. Please align the comment with the new exception policy to avoid reintroducing the deprecated type.
Proposed update:
- // TODO: Add ErrorCode class for `stdexcept` errors and switch to use TraceableException + // TODO: Introduce an ErrorCode for `stdexcept` errors if needed; TraceableException has been removed.
48-49: Consider marking move operations noexcept.Declaring the move ctor/assign as noexcept improves interoperability with standard containers and communicates strong move guarantees (unique_ptr already provides noexcept moves).
Proposed change:
- Array(Array&&) = default; - auto operator=(Array&&) -> Array& = default; + Array(Array&&) noexcept = default; + auto operator=(Array&&) noexcept -> Array& = default;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/unit-tests.yaml(1 hunks)src/ystdlib/containers/Array.hpp(1 hunks)src/ystdlib/containers/test/test_Array.cpp(7 hunks)src/ystdlib/error_handling/test/test_ErrorCode.cpp(1 hunks)src/ystdlib/error_handling/test/types.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
src/ystdlib/containers/Array.hppsrc/ystdlib/error_handling/test/test_ErrorCode.cppsrc/ystdlib/error_handling/test/types.cppsrc/ystdlib/containers/test/test_Array.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#72
File: cmake/Toolchains/utils.cmake:30-41
Timestamp: 2025-08-12T18:00:59.358Z
Learning: The ystdlib-cpp project maintains C++20 support and requires minimum compiler versions of Clang 15, Apple Clang 15, and GNU 11 to ensure compatibility with all C++20 features used in the codebase.
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
📚 Learning: 2025-03-27T22:57:03.031Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#0
File: :0-0
Timestamp: 2025-03-27T22:57:03.031Z
Learning: In unit tests for ystdlib-cpp, `REQUIRE_FALSE` is preferred over `REQUIRE(!condition)` for its explicit nature when checking that conditions are false.
Applied to files:
src/ystdlib/error_handling/test/test_ErrorCode.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-ystdlib-cpp (macos-15, release)
- GitHub Check: build-ystdlib-cpp (macos-15, debug)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04, debug)
- GitHub Check: build-ystdlib-cpp (macos-14, release)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04, release)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04, release)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04, debug)
- GitHub Check: build-ystdlib-cpp (macos-14, debug)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (10)
.github/workflows/unit-tests.yaml (2)
46-46: Step name update aligns with relaxed compiler policyRenaming to reference LLVM Clang 15 (for Velox) is consistent with the project’s new minimum compiler requirement and helps future readers understand the rationale.
45-50: CMake toolchain selection verified
We confirmed that inCMakeLists.txt:
setup_toolchains()is called on line 6, beforeproject()on line 8.cmake/Toolchains/llvm-clang-15-toolchain.cmakeexplicitly sets
CMAKE_C_COMPILERandCMAKE_CXX_COMPILERto the brew-installed LLVM 15 binaries.As a result, CMake deterministically picks LLVM 15 without any
CC/CXXoverrides.If you’d still like to ensure non-CMake steps (e.g. direct
clangcalls) use the brew LLVM 15, you can prepend itsbintoPATH:- - if: "'macos-14' == matrix.os" - name: "Install macOS 14 deps: coreutils (for md5sum) and LLVM Clang 15 (for Velox)" - run: |- - brew install coreutils - brew install llvm@15 + - if: "'macos-14' == matrix.os" + name: "Install macOS 14 deps: coreutils (for md5sum) and LLVM Clang 15 (for Velox)" + run: |- + brew update + brew install coreutils + brew install llvm@15 + echo "$(brew --prefix llvm@15)/bin" >> "$GITHUB_PATH"src/ystdlib/containers/Array.hpp (1)
36-41: Switch to std::copy is appropriate for Clang 15 (ranges-free) while preserving semantics.This keeps the same behaviour but avoids ranges dependencies that are shaky on Clang 15/libc++. LGTM.
src/ystdlib/containers/test/test_Array.cpp (7)
68-74: Good replacement of ranges::equal with iterator-based std::equal and a comparator.This preserves the original semantics without ranges and is portable to Clang 15.
93-93: Iterator-based equality check is correct and clearer for Clang 15.Comparing both ranges explicitly via 4-iterator overload ensures equal length and contents. LGTM.
102-104: Copy and equality checks without ranges are correct.Using std::copy and std::equal over begin/end is the right call here. LGTM.
114-119: Move test remains sound after switching to std::equal.The 4-iterator overload is appropriate and keeps the same verification semantics. LGTM.
153-155: Replacing ranges::for_each with std::for_each is correct.The lambda checks default zero-initialisation for fundamental types; matches Array’s constructor behaviour. LGTM.
168-168: Iterator-based sequence comparison is equivalent and portable.Dropping ranges here aligns with the toolchain requirements. LGTM.
184-190: Comparator-based std::equal is a faithful replacement and clearer than transformed views.Readability improves and it avoids ranges dependencies while preserving behaviour. LGTM.
Bill-hbrhbr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/ystdlib/error_handling/test/test_ErrorCode.cpp (1)
55-57: Use REQUIRE_FALSE for explicit negative assertion (team preference)Per retrieved team learnings, prefer explicit negative assertions in tests. Replace the inequality check with REQUIRE_FALSE for clarity.
- for (auto const& none_failure_condition : cNoneFailureConditions) { - REQUIRE(failure_error_code != none_failure_condition); - } + for (auto const& none_failure_condition : cNoneFailureConditions) { + REQUIRE_FALSE(failure_error_code == none_failure_condition); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
cmake/Toolchains/llvm-clang-15-toolchain.cmake(1 hunks)cmake/Toolchains/utils.cmake(1 hunks)src/ystdlib/error_handling/test/test_ErrorCode.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
src/ystdlib/error_handling/test/test_ErrorCode.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#72
File: cmake/Toolchains/utils.cmake:30-41
Timestamp: 2025-08-12T18:00:59.358Z
Learning: The ystdlib-cpp project maintains C++20 support and requires minimum compiler versions of Clang 15, Apple Clang 15, and GNU 11 to ensure compatibility with all C++20 features used in the codebase.
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
📚 Learning: 2025-03-27T22:57:03.031Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#0
File: :0-0
Timestamp: 2025-03-27T22:57:03.031Z
Learning: In unit tests for ystdlib-cpp, `REQUIRE_FALSE` is preferred over `REQUIRE(!condition)` for its explicit nature when checking that conditions are false.
Applied to files:
src/ystdlib/error_handling/test/test_ErrorCode.cpp
📚 Learning: 2025-02-28T05:04:33.616Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
Applied to files:
cmake/Toolchains/llvm-clang-15-toolchain.cmakecmake/Toolchains/utils.cmake
📚 Learning: 2025-02-28T05:04:33.616Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr maintains a maximum line length of 100 characters in CMake files and uses backslash line continuations to maintain this limit.
Applied to files:
cmake/Toolchains/llvm-clang-15-toolchain.cmakecmake/Toolchains/utils.cmake
📚 Learning: 2025-08-12T18:00:59.358Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#72
File: cmake/Toolchains/utils.cmake:30-41
Timestamp: 2025-08-12T18:00:59.358Z
Learning: The ystdlib-cpp project maintains C++20 support and requires minimum compiler versions of Clang 15, Apple Clang 15, and GNU 11 to ensure compatibility with all C++20 features used in the codebase.
Applied to files:
cmake/Toolchains/utils.cmake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-ystdlib-cpp (macos-14, debug)
- GitHub Check: build-ystdlib-cpp (macos-14, release)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04, release)
- GitHub Check: build-ystdlib-cpp (macos-15, debug)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04, debug)
- GitHub Check: build-ystdlib-cpp (macos-15, release)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04, release)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04, debug)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
src/ystdlib/error_handling/test/test_ErrorCode.cpp (1)
52-54: LGTM: Switched to range-based for loop for clarityThis improves readability over the previous algorithm-based approach in tests. Nice cleanup.
cmake/Toolchains/utils.cmake (1)
37-41: Intentional exclusion of MSVC in validate_compiler_versions
Thevalidate_compiler_versionsfunction incmake/Toolchains/utils.cmakeonly handles AppleClang, Clang and GNU – all other compilers (including MSVC) hit theFATAL_ERROR. This matches our project policy (C++20 support on Clang/AppleClang ≥15 and GNU ≥11). No change is required.If Windows/MSVC support is needed in future, please add an
elseif("MSVC" STREQUAL "${CMAKE_CXX_COMPILER_ID}")branch with the minimum version (e.g. MSVC ≥ 19.36 with/std:c++20).
Bill-hbrhbr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cmake/Toolchains/llvm-clang-15-toolchain.cmake (1)
23-27: Fix multi-line FATAL_ERROR formatting (avoid extra space, preserve readability)The current split into two quoted strings yields a double space between
'brewandinstall. Use a bracket argument to preserve formatting and avoid accidental spacing issues.Apply this diff:
- message( - FATAL_ERROR - "Failed to locate LLVM v15 using Homebrew. Please ensure llvm@15 is installed: 'brew" - " install llvm@15'" - ) + message( + FATAL_ERROR + [=[ +Failed to locate LLVM v15 using Homebrew. Please ensure llvm@15 is installed: +'brew install llvm@15' + ]=] + )cmake/Toolchains/utils.cmake (1)
14-20: Prefer CMAKE_CURRENT_LIST_DIR for robust relative pathing (optional)Using CMAKE_CURRENT_LIST_DIR makes the toolchain file path independent of the including directory. You’ve set the cache type to FILEPATH (great); swapping to LIST_DIR would further harden this.
- set(CMAKE_TOOLCHAIN_FILE - "${CMAKE_CURRENT_SOURCE_DIR}/cmake/Toolchains/llvm-clang-15-toolchain.cmake" + set(CMAKE_TOOLCHAIN_FILE + "${CMAKE_CURRENT_LIST_DIR}/llvm-clang-15-toolchain.cmake" CACHE FILEPATH "Toolchain file" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
cmake/Toolchains/llvm-clang-15-toolchain.cmake(1 hunks)cmake/Toolchains/utils.cmake(1 hunks)tools/yscope-dev-utils(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#72
File: cmake/Toolchains/utils.cmake:30-41
Timestamp: 2025-08-12T18:00:59.401Z
Learning: The ystdlib-cpp project maintains C++20 support and requires minimum compiler versions of Clang 15, Apple Clang 15, and GNU 11 to ensure compatibility with all C++20 features used in the codebase.
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
📚 Learning: 2025-02-28T05:04:33.616Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
Applied to files:
cmake/Toolchains/llvm-clang-15-toolchain.cmakecmake/Toolchains/utils.cmake
📚 Learning: 2025-02-28T05:04:33.616Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr maintains a maximum line length of 100 characters in CMake files and uses backslash line continuations to maintain this limit.
Applied to files:
cmake/Toolchains/llvm-clang-15-toolchain.cmakecmake/Toolchains/utils.cmake
📚 Learning: 2025-08-12T18:00:59.401Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#72
File: cmake/Toolchains/utils.cmake:30-41
Timestamp: 2025-08-12T18:00:59.401Z
Learning: The ystdlib-cpp project maintains C++20 support and requires minimum compiler versions of Clang 15, Apple Clang 15, and GNU 11 to ensure compatibility with all C++20 features used in the codebase.
Applied to files:
cmake/Toolchains/utils.cmake
📚 Learning: 2025-06-06T15:01:53.987Z
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
Applied to files:
cmake/Toolchains/utils.cmake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04, debug)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04, release)
- GitHub Check: build-ystdlib-cpp (macos-14, debug)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04, release)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04, debug)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (7)
tools/yscope-dev-utils (2)
1-1: Submodule bump looks aligned with the PR’s Clang 15 objective.Updating yscope-dev-utils here makes sense given the intent to relax clang-tidy checks for Clang/AppleClang 15. No issues with the pointer update itself.
1-1: Clang-tidy configuration correctly relaxes checks for Clang/AppleClang 15The exported
.clang-tidyin tools/yscope-dev-utils@e121a57 disables themodernize-use-rangescheck (and other heavy rules) specifically to maintain compatibility with C++20 on Clang 15:
- Disabled checks in exports/lint-configs/.clang-tidy:
-modernize-use-ranges-bugprone-easily-swappable-parameters-portability-template-virtual-member-function-readability-identifier-length-readability-named-parameter-readability-simplify-boolean-exprThe in-file comment explicitly cites Clang 15 support for disabling ranges. No further changes are required.
cmake/Toolchains/llvm-clang-15-toolchain.cmake (3)
6-11: Good safety check helper for toolchain binariesValidates existence and propagates variables cleanly. Using a helper keeps call sites concise.
15-21: Brew prefix discovery is fine for macOS-only usageThe Homebrew lookup is straightforward and appropriate for this macOS-specific toolchain file.
30-33: Setting compilers/ar/ranlib via validated paths looks solidGiven the upfront existence checks, assigning these variables here is reliable for the toolchain context.
cmake/Toolchains/utils.cmake (2)
24-36: Compiler minimums align with your stated C++20 baselineMinimum versions for AppleClang/Clang 15 and GNU 11 reflect the project’s C++20 requirements. No issues.
42-47: Good fatal message; formatting avoids backslash continuationsSplitting the message across arguments avoids the backslash pitfall and stays within line-length preferences.
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
.github/workflows/unit-tests.yaml (1)
50-56: Run unit tests only on debug matrix to avoid redundant work on releaseYou always run debug tests even for the release matrix. Gate the tests by build_type to save CI time.
Apply this diff:
- - name: "Run unit tests and examples" + - name: "Run unit tests and examples" # Currently unit tests rely on cassert and fail to compile in release mode. run: |- - task test:run-debug + if [ "${{ matrix.build_type }}" = "debug" ]; then + task test:run-debug + fi task examples:build-${{matrix.build_type}} ./build/examples/${{matrix.build_type}}/linking-tests
♻️ Duplicate comments (3)
.github/workflows/unit-tests.yaml (1)
45-49: LGTM: simplify macOS 14 setup by dropping LLVM 16 installRelying on the runner’s built-in AppleClang 15 and only installing coreutils is correct for macOS 14.
cmake/Toolchains/llvm-clang-15-toolchain.cmake (1)
22-27: Use a bracketed multi-line string for the fatal message (cleaner, no awkward quoting)Improves readability and avoids accidental formatting issues.
Apply this diff:
if(NOT 0 EQUAL BREW_RESULT) message( FATAL_ERROR - "Failed to locate LLVM v15 using Homebrew. Please ensure llvm@15 is installed: 'brew" - " install llvm@15'" + [=[ +Failed to locate LLVM v15 using Homebrew. Please ensure llvm@15 is installed: +'brew install llvm@15' + ]=] ) endif()cmake/Toolchains/utils.cmake (1)
1-6: Add include guard to prevent redefinition on multiple includesPrevents accidental re-inclusion and function redefinition.
Apply this diff:
# This file contains utility functions for setting up toolchains and validating toolchain versions # to ensure compatibility with the C++20 features required by the project. +include_guard(GLOBAL) + # Sets up the appropriate toolchain file based on the host system. function(setup_toolchains)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/unit-tests.yaml(1 hunks)cmake/Toolchains/llvm-clang-15-toolchain.cmake(1 hunks)cmake/Toolchains/utils.cmake(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#72
File: cmake/Toolchains/utils.cmake:30-41
Timestamp: 2025-08-12T18:00:59.401Z
Learning: The ystdlib-cpp project maintains C++20 support and requires minimum compiler versions of Clang 15, Apple Clang 15, and GNU 11 to ensure compatibility with all C++20 features used in the codebase.
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
📚 Learning: 2025-08-12T18:00:59.401Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#72
File: cmake/Toolchains/utils.cmake:30-41
Timestamp: 2025-08-12T18:00:59.401Z
Learning: The ystdlib-cpp project maintains C++20 support and requires minimum compiler versions of Clang 15, Apple Clang 15, and GNU 11 to ensure compatibility with all C++20 features used in the codebase.
Applied to files:
cmake/Toolchains/utils.cmake
📚 Learning: 2025-02-28T05:04:33.616Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
Applied to files:
cmake/Toolchains/utils.cmakecmake/Toolchains/llvm-clang-15-toolchain.cmake
📚 Learning: 2025-02-28T05:04:33.616Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr maintains a maximum line length of 100 characters in CMake files and uses backslash line continuations to maintain this limit.
Applied to files:
cmake/Toolchains/utils.cmakecmake/Toolchains/llvm-clang-15-toolchain.cmake
📚 Learning: 2025-06-06T15:01:53.987Z
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
Applied to files:
cmake/Toolchains/utils.cmake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build-ystdlib-cpp (macos-14, release)
- GitHub Check: build-ystdlib-cpp (macos-15, release)
- GitHub Check: build-ystdlib-cpp (macos-15, debug)
🔇 Additional comments (1)
cmake/Toolchains/utils.cmake (1)
29-49: Compiler version validation aligns with stated C++20 floorThe per-compiler minimums (AppleClang/Clang 15, GNU 11) match the project’s C++20 support policy. Messaging is clear and actionable.
There was a problem hiding this comment.
For the PR title & body, how about:
build(cmake): Lower minimum Clang version for Velox compatibility:
- Lower LLVM/Apple Clang minimum version from 16 to 15.
- Remove `TraceableException` which used C++20 features unsupported by Clang 15.
- Update `yscope-dev-utils` to disable `clang-tidy` checks that require C++20 features unsupported by Clang 15.
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmake/Toolchains/utils.cmake (1)
1-3: Add an include guard to prevent redefinition on multiple includesCheap safety against accidental double-inclusion.
Apply this diff:
# This file contains utility functions for validating toolchain versions to ensure compatibility # with the C++20 features required by the project. +include_guard(GLOBAL) +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/unit-tests.yaml(1 hunks)CMakeLists.txt(1 hunks)cmake/Toolchains/utils.cmake(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#72
File: cmake/Toolchains/utils.cmake:30-41
Timestamp: 2025-08-12T18:00:59.401Z
Learning: The ystdlib-cpp project maintains C++20 support and requires minimum compiler versions of Clang 15, Apple Clang 15, and GNU 11 to ensure compatibility with all C++20 features used in the codebase.
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
📚 Learning: 2025-08-12T18:00:59.401Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#72
File: cmake/Toolchains/utils.cmake:30-41
Timestamp: 2025-08-12T18:00:59.401Z
Learning: The ystdlib-cpp project maintains C++20 support and requires minimum compiler versions of Clang 15, Apple Clang 15, and GNU 11 to ensure compatibility with all C++20 features used in the codebase.
Applied to files:
cmake/Toolchains/utils.cmakeCMakeLists.txt
📚 Learning: 2025-02-28T05:04:33.616Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr uses a maximum line length of 100 characters in CMake files and prefers to use backslash line continuations to maintain this limit.
Applied to files:
cmake/Toolchains/utils.cmake
📚 Learning: 2025-02-28T05:04:33.616Z
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: Bill-hbrhbr maintains a maximum line length of 100 characters in CMake files and uses backslash line continuations to maintain this limit.
Applied to files:
cmake/Toolchains/utils.cmake
📚 Learning: 2025-06-06T15:01:53.987Z
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
Applied to files:
cmake/Toolchains/utils.cmake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04, debug)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04, debug)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04, release)
- GitHub Check: build-ystdlib-cpp (macos-15, release)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04, release)
- GitHub Check: build-ystdlib-cpp (macos-14, release)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (6)
.github/workflows/unit-tests.yaml (2)
46-46: Step name simplification looks goodName is concise and clear.
51-52: Formatting nit resolvedThanks for adding the empty line before the comment to match the guideline.
cmake/Toolchains/utils.cmake (2)
7-13: Version matrix aligns with stated C++20 baselineAppleClang/Clang ≥ 15 and GNU ≥ 11 matches your documented C++20 support stance.
19-25: Good fatal error messagingSplitting the message avoids stray backslashes and keeps line length reasonable.
CMakeLists.txt (2)
8-8: Including Toolchains/utils via CMAKE_MODULE_PATH is correctThe module path prepend ensures findability of cmake/Toolchains/utils.cmake.
11-11: validate_compiler_versions() placement is correctCalled after project(... LANGUAGES CXX), so compiler ID/version variables are available. Centralizing validation here is cleaner than per-file checks.
| - if: "'macos-14' == matrix.os" | ||
| name: "Install macOS 14 deps: coreutils (for md5sum) and Apple Clang 16 (for C++20)" | ||
| name: "Install coreutils (for md5sum)" | ||
| run: |- | ||
| brew install coreutils | ||
| brew install llvm@16 | ||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure md5sum is on PATH after installing coreutils on macOS
Homebrew’s coreutils installs md5sum under gnubin (not on PATH by default). If any step calls md5sum, it will fail unless you add gnubin to PATH or use gmd5sum explicitly.
Apply this diff to export gnubin to PATH robustly:
- run: |-
- brew install coreutils
+ run: |-
+ brew install coreutils
+ # Ensure md5sum is available on PATH (coreutils installs it under gnubin).
+ echo "$(brew --prefix coreutils)/libexec/gnubin" >> "$GITHUB_PATH"To verify whether md5sum (or gmd5sum) is actually used in the repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for md5sum usage..."
rg -n -C2 -g '!**/node_modules/**' -P '\b(g?md5sum)\b' || echo "No md5sum usage found."Length of output: 200
No md5sum usage detected – coreutils install step may be unnecessary
I searched the repo for any invocations of md5sum or gmd5sum and found none. As a result:
- You can safely remove the macOS
brew install coreutilsstep if it’s not actually used. - If you prefer to keep installing coreutils “just in case,” append its gnubin directory to your PATH to future-proof against any calls to
md5sum:- if: "'macos-14' == matrix.os" name: Install coreutils (for md5sum) run: |- brew install coreutils echo "$(brew --prefix coreutils)/libexec/gnubin" >> "$GITHUB_PATH"
🤖 Prompt for AI Agents
.github/workflows/unit-tests.yaml around lines 45 to 49: the workflow installs
coreutils on macOS even though no md5sum usage was found; either remove this
macOS-specific brew install step entirely (delete those lines) or keep the
install but append the coreutils gnubin directory to the PATH by adding a
command after brew install that writes the gnubin path to GITHUB_PATH so future
invocations of md5sum (or other GNU tools) resolve correctly.
2 participants
TraceableExceptionwhich used C++20 features unsupported by Clang 15.yscope-dev-utilsto disableclang-tidychecks that require C++20 features unsupported by Clang 15.Description
With the integration of Velox, which will depend on
ystdlib, our minimum compiler requirements must align withVelox's build constraints.Velox currently requires compiler versions of GNU11, Clang 15 and AppleClang15, so we must relax
ystdlibconstraints from LLVM/Apple Clang 16 down to 15 as well.The compiler validation process follows the already approved practice in CLP.
The changes required to make
ystdlibwork with LLVM/Apple Clang 15 are:TraceableExceptionfromerror_handlingdue to its use ofstd::source_locationstd::rangesand remove clang tidy check that enforcesstd::rangesoptimizationsChecklist
breaking change.
Validation performed
ystdlibunittests for building and installation runs successfully.Summary by CodeRabbit
Breaking Changes
Chores
Refactor
Tests
Documentation