-
Notifications
You must be signed in to change notification settings - Fork 71
Fix compatibility with C++17, GCC 13 and CMake 4 #112
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.
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
Conversation
Support for < 3.5 was dropped and 3.10 deprecated in CMake 4
Reviewer's Guide by SourceryThis pull request addresses compatibility issues with C++17, GCC 13, and CMake 4. It updates the minimum CMake version, suppresses a GCC warning, updates the checkout action version in the test workflow, and removes a custom output operator for File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Flamefire - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining why the CMake version is being bumped to 3.16.
- It might be good to use
actions/checkout@v3instead ofactions/checkout@v4for better compatibility.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 13) | ||
| add_definitions(-Wno-dangling-reference) | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Explain the purpose of -Wno-dangling-reference.
A comment explaining why this flag is necessary for GNU compilers version 13 and above would be helpful.
| if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 13) | |
| add_definitions(-Wno-dangling-reference) | |
| endif() | |
| if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 13) | |
| # Suppress warnings about dangling references, which can occur in template-heavy code | |
| # and are treated as errors in GNU compiler version 13 and above. | |
| add_definitions(-Wno-dangling-reference) | |
| endif() |
Summary by Sourcery
Update project compatibility with modern C++ standards, CMake, and compiler versions
Bug Fixes:
Enhancements:
Build:
CI: