Upgrade to C++26 and enable strict compilation (-Wall -Werror)#89
Upgrade to C++26 and enable strict compilation (-Wall -Werror)#89
Conversation
- Bump C++ standard from C++23 to C++26 in CMakeLists.txt - Enable -Wall and -Werror compiler flags for stricter builds - Foundation for comprehensive warning fixes in follow-up PRs Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary of ChangesHello @Luohaothu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to enhance the project's code quality and future-proof its development by upgrading the C++ standard to C++26 and enforcing strict compiler warnings. This foundational change will ensure that new code adheres to modern C++ practices and that any potential issues are caught early in the development cycle, setting the stage for subsequent warning-fix pull requests. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThe CMake header-only non-module comment was updated to reference C++26 (from C++20), and INTERFACE compiler warning flags were added to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request upgrades the project to C++26 and enables strict compiler warnings. The changes correctly update the C++ standard version. However, there is a critical issue in the implementation of the compiler warning flags in src/CMakeLists.txt. The new code uses an undefined CMake variable, which will lead to build failures in the header-only configuration. I have provided a detailed comment with a code suggestion to resolve this issue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff28310f56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/CMakeLists.txt`:
- Around line 125-130: The CMakeLists uses OPFLOW_TARGET_SCOPE in
target_compile_options(opflow ${OPFLOW_TARGET_SCOPE} ...) but
OPFLOW_TARGET_SCOPE is never defined; define it (e.g., set(OPFLOW_TARGET_SCOPE
INTERFACE) or PUBLIC/PRIVATE as appropriate) before its use or remove the
variable and substitute the correct scope keyword directly where
target_compile_options is called (refer to OPFLOW_TARGET_SCOPE and the
target_compile_options(opflow ...) invocation to locate the change).
- Around line 45-50: The project adds an INTERFACE target opflow and requires
C++26 via target_compile_features(opflow INTERFACE cxx_std_26), so update CI to
guarantee compilers that support C++26 (GCC >=14, Clang >=18, MSVC >=17.13) or
add an explicit pre-check in CMake to detect compiler versions and emit a clear
error; specifically adjust your CI job definitions (toolchain images/versions)
to use the stated minimums and/or implement a CMake guard around
add_library/opflow and target_compile_features that tests CMAKE_CXX_COMPILER_ID
and CMAKE_CXX_COMPILER_VERSION and fails with a helpful message if the compiler
is too old.
🧹 Nitpick comments (1)
src/CMakeLists.txt (1)
123-137: Avoid exporting-Werror//WXto consumers by default.Using
-Werror(or/WX) on an INTERFACE/PUBLIC target can break downstream builds due to their own warnings. Consider limiting Werror to build-interface or making it opt‑in.Proposed adjustment (limit Werror to build interface)
if(CMAKE_CXX_COMPILER_ID MATCHES "GNU" OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") target_compile_options(opflow ${OPFLOW_TARGET_SCOPE} -Wall -Wextra - -Werror + "$<$<BUILD_INTERFACE:-Werror>>" ) endif() # MSVC-specific warnings if(MSVC) target_compile_options(opflow ${OPFLOW_TARGET_SCOPE} /W4 - /WX + "$<$<BUILD_INTERFACE:/WX>>" ) endif()
Summary
Upgrade C++ standard to C++26 and enable strict compiler warnings to improve code quality.
Changes
Test Plan
Dependencies
This PR should be merged before:
🤖 Generated with Claude Code
Summary by CodeRabbit