Enable C++20 standard#1285
Open
OldDev78 wants to merge 12 commits into
Open
Conversation
|
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:
WalkthroughRaises the required C++ standard from C++17 to C++20 in ChangesC++20 Build Gating and ThorVG Namespace Qualification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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
🧹 Nitpick comments (1)
SConstruct (1)
696-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove stale VS2017/VS2019 messaging after requiring VS2022.
The MSVC gate now exits for
cc_version_major < 17, so the VS2017/VS2019 comment is outdated and thecc_version_major < 16warning is unreachable.Proposed cleanup
elif env.msvc: - # Ensure latest minor builds of Visual Studio 2017/2019. - # https://github.com/godotengine/godot/pull/94995#issuecomment-2336464574 + # Require Visual Studio 2022 or newer for C++20 support. if cc_version_major < 17: print_error( "Detected Visual Studio version older than VS2022, which has bugs "else: - # MSVC started offering C standard support with Visual Studio 2019 16.8 env.Prepend(CXXFLAGS=["/std:c++20"]) - if cc_version_major < 16: - print_warning("Visual Studio 2017 or older cannot specify a C-Standard.")Also applies to: 820-823
🤖 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 `@SConstruct` around lines 696 - 697, Update the MSVC version gating comments and warning text in SConstruct to match the current VS2022-only requirement. The stale “latest minor builds of Visual Studio 2017/2019” note and the `cc_version_major < 16` warning are no longer reachable once the `cc_version_major < 17` check is in place, so remove or rewrite those messages in the relevant MSVC setup block to reflect the VS2022 gate and keep the logic consistent.
🤖 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.
Inline comments:
In `@SConstruct`:
- Around line 663-665: The GCC version check in SConstruct is now gated at 12,
but the error text in the print_error path still refers to supported versions
starting at 9. Update the diagnostic message in the same GCC-version validation
block to match the new minimum supported GCC 12, keeping it consistent with the
version check and any related wording in the version-gate logic.
---
Nitpick comments:
In `@SConstruct`:
- Around line 696-697: Update the MSVC version gating comments and warning text
in SConstruct to match the current VS2022-only requirement. The stale “latest
minor builds of Visual Studio 2017/2019” note and the `cc_version_major < 16`
warning are no longer reachable once the `cc_version_major < 17` check is in
place, so remove or rewrite those messages in the relevant MSVC setup block to
reflect the VS2022 gate and keep the logic consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 216ca75a-7c32-46f8-97fd-503ac4814cf1
📒 Files selected for processing (3)
SConstructthirdparty/thorvg/src/loaders/svg/tvgSvgSceneBuilder.cppthirdparty/thorvg/src/renderer/sw_engine/tvgSwFill.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In @.github/workflows/linux_builds.yml:
- Around line 150-152: The GCC 12 toolchain override is only applied to the main
Compilation step, so the redot-cpp build can still use the default compiler.
Update the workflow job for the build steps so `CC` and `CXX` are available to
both Compilation and Compilation (redot-cpp), either by moving the env settings
to the job level or duplicating them on the redot-cpp step; use the existing
Compilation and Compilation (redot-cpp) step definitions as the anchors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37d3f9d6-fdb0-43de-a96e-55c8373d5d38
📒 Files selected for processing (1)
.github/workflows/linux_builds.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/linux_builds.yml (1)
158-167: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove the unsupported
type:keys from.github/actions/redot-cpp-build/action.yml
Composite-action inputs are strings only, sotype:is ignored here and should be dropped to avoid confusion.🤖 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 @.github/workflows/linux_builds.yml around lines 158 - 167, Remove the unsupported type declarations from the redot-cpp composite action inputs in the action.yml used by the Compilation (redot-cpp) workflow step, since composite-action inputs are string-only. Update the inputs definition in redot-cpp-build/action.yml to keep only the input names and defaults, and ensure the workflow step that uses redot-cpp-build still passes the same values via with: without relying on type annotations.Source: Linters/SAST tools
🤖 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.
Inline comments:
In @.github/workflows/linux_builds.yml:
- Line 91: The workflow is hard-coding GCC 12 for every Compilation matrix job,
which breaks the LLVM/TSAN row that should validate the Clang path. Update the
compiler setup in the linux_builds workflow to be matrix-driven using the
existing job fields like use_llvm and linker: keep gcc-12/g++-12 only for
non-LLVM rows, and for the TSAN/LLVM row install and export clang-16/clang++-16
instead. Make the same conditional selection wherever CC/CXX are assigned so the
LLVM row continues to exercise the correct toolchain.
---
Nitpick comments:
In @.github/workflows/linux_builds.yml:
- Around line 158-167: Remove the unsupported type declarations from the
redot-cpp composite action inputs in the action.yml used by the Compilation
(redot-cpp) workflow step, since composite-action inputs are string-only. Update
the inputs definition in redot-cpp-build/action.yml to keep only the input names
and defaults, and ensure the workflow step that uses redot-cpp-build still
passes the same values via with: without relying on type annotations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9475094-e7c4-4e26-9a51-8d7de396b765
📒 Files selected for processing (1)
.github/workflows/linux_builds.yml
OldDev78
force-pushed
the
cpp20
branch
2 times, most recently
from
3251a38 to
3c50f83
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enables C++20 standard, and bumps up: minimum gcc/g++ version to 12, minimum VS to 2022, and clang to 16.
Also upgrades the Ubuntu version used for Linux builds to 24.04 (previously released LTS), in order to allow CI to use a g++ of version >= 12.
Includes multiple compilation fixes.
Summary by CodeRabbit