Require C++20#2410
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates build, formatting, and test helper code to use C++20-oriented configuration and formatting. ChangesC++20 Standard Upgrade
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/CMakeLists.txt (1)
133-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate the C++20 requirement to downstream consumers.
The
rmmtarget setsCXX_STANDARD 20for its own compilation, but this property does not propagate to consumers linking againstrmm::rmm. To ensure downstream projects inherit the C++20 requirement, addtarget_compile_features(rmm PUBLIC cxx_std_20)after theset_target_properties()call.Suggested patch
set_target_properties( rmm PROPERTIES BUILD_RPATH "\$ORIGIN" INSTALL_RPATH "\$ORIGIN" CXX_STANDARD 20 CXX_STANDARD_REQUIRED ON CXX_VISIBILITY_PRESET hidden POSITION_INDEPENDENT_CODE ON INTERFACE_POSITION_INDEPENDENT_CODE ON) + +target_compile_features(rmm PUBLIC cxx_std_20)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/CMakeLists.txt` around lines 133 - 141, The rmm target sets its own CXX_STANDARD but doesn't propagate C++20 to consumers; add a call to target_compile_features for the rmm target to export the requirement (i.e., call target_compile_features(rmm PUBLIC cxx_std_20)) immediately after the set_target_properties(...) block so downstream targets linking to rmm::rmm inherit the C++20 requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/CMakeLists.txt`:
- Around line 133-141: The rmm target sets its own CXX_STANDARD but doesn't
propagate C++20 to consumers; add a call to target_compile_features for the rmm
target to export the requirement (i.e., call target_compile_features(rmm PUBLIC
cxx_std_20)) immediately after the set_target_properties(...) block so
downstream targets linking to rmm::rmm inherit the C++20 requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7412acc7-d45b-44ca-b3d0-c56a634e7251
📒 Files selected for processing (2)
cpp/CMakeLists.txtcpp/examples/basic/CMakeLists.txt
lamarrr
left a comment
There was a problem hiding this comment.
LGTM, thanks for working on this!
|
/merge |
Description
This updates the RMM library target and basic example to require C++20.
Closes #2034
Checklist