Allow C17 for newer CMake versions#1958
Conversation
WalkthroughAdds C17 and C23 to allowed C standards when CMake > 3.20 and expands the CI matrix to test C17/C23 across Ubuntu (GCC/Clang) and Windows (MSVC); existing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CMakeLists.txt (1)
24-26: Approve: C17 gate is correct; update documentation and CI to mention/test C17
- README.md (line 19): change “Modern C11 syntax” to “Modern C11/C17 syntax”
- CMakeLists.txt (lines 11–15): default C11 comment—add a note that C17 is now supported
- CI workflows (e.g. .github/workflows/*): add a matrix entry for CMAKE_C_STANDARD=17
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 (1)
CMakeLists.txt(1 hunks)
CMakeLists.txt
Outdated
| set(CMAKE_C_EXTENSIONS OFF) # Boolean specifying whether compiler specific extensions are requested | ||
| endif() | ||
| set(VALID_C_STANDARDS "99" "11") | ||
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21) |
There was a problem hiding this comment.
VERSION_GREATER_EQUAL - cmake >= 3.7, but zlib-ng require cmake 3.5.1+
I would suggest removing the C standard version check.
There was a problem hiding this comment.
Ah yes, it should be VERSION_GREATER 3.20 to work with 3.5.1 as well.
There was a problem hiding this comment.
I couldn't find anything on why the version check is there but assumed that it's there for some reason, so I didn't want to just take it out myself.
|
I don't see any reason why we shouldn't allow compiling with more recent C standard as we have already decided that we support at least 3 most recent compiler major versions. Removing features isn't an issue if there is a replacement that works at least as good as the currently used function or feature. In most cases only extra addition we should require is adding the new C standard in new CI workflow, so we immediately know if there is breaking change. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.github/workflows/cmake.yml (5)
89-94: Fail fast if C17 isn’t honored: add CMAKE_C_STANDARD_REQUIRED=ON.Prevents silent fallback to an older standard on some toolchains/CMake combos.
- cmake-args: -DCMAKE_C_STANDARD=17 + cmake-args: -DCMAKE_C_STANDARD=17 -DCMAKE_C_STANDARD_REQUIRED=ON
95-100: Treat C23 as experimental and require the standard.C23 support is still evolving across compilers/CMake; keep the signal without failing the whole CI and enforce the requested standard.
- name: Ubuntu GCC C23 os: ubuntu-latest compiler: gcc cxx-compiler: g++ - cmake-args: -DCMAKE_C_STANDARD=23 + cmake-args: -DCMAKE_C_STANDARD=23 -DCMAKE_C_STANDARD_REQUIRED=ON + experimental: trueAlso add at the job level (outside this hunk) to honor experimental jobs:
continue-on-error: ${{ matrix.experimental || false }}
437-443: Apply the same “require standard” guard for Clang C17.Avoids unintended fallback when CMake/Clang mapping changes.
- cmake-args: -DCMAKE_C_STANDARD=17 + cmake-args: -DCMAKE_C_STANDARD=17 -DCMAKE_C_STANDARD_REQUIRED=ON
444-450: Mark Clang C23 as experimental and require the standard.Clang-15’s C23 coverage is partial; keep CI green while still exercising it.
- name: Ubuntu Clang C23 os: ubuntu-latest compiler: clang-15 cxx-compiler: clang++-15 - cmake-args: -DCMAKE_C_STANDARD=23 + cmake-args: -DCMAKE_C_STANDARD=23 -DCMAKE_C_STANDARD_REQUIRED=ON packages: clang-15 llvm-15 llvm-15-tools + experimental: trueRemember to set (outside this hunk):
continue-on-error: ${{ matrix.experimental || false }}
544-548: Optional: require C17 on MSVC too.MSVC maps C standards differently, but this keeps behavior consistent across jobs.
- cmake-args: -G "Visual Studio 17 2022" -A x64 -T v143 -DCMAKE_C_STANDARD=17 + cmake-args: -G "Visual Studio 17 2022" -A x64 -T v143 -DCMAKE_C_STANDARD=17 -DCMAKE_C_STANDARD_REQUIRED=ON
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 (2)
.github/workflows/cmake.yml(3 hunks)CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
|
I added some CI workflows for C17 and C23, please adjust them as necessary.
Edit: I just noticed that |
|
Latest MSVC version is too old to have C23 support, but there is chattering about new MSVC release near November 2025... We should see added features when first public betas are available... What comes to the added CI workflows, we can comment on them after they have been approved and finished running. Basically we need to look for any failed tests, be it a busy or idle loop, crash, checksum mismatch, or data corruption. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1958 +/- ##
===========================================
+ Coverage 81.58% 81.60% +0.02%
===========================================
Files 163 163
Lines 14073 14074 +1
Branches 3165 3166 +1
===========================================
+ Hits 11481 11485 +4
+ Misses 1625 1612 -13
- Partials 967 977 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@alxvth Could you please rebase this PR? Bunch of CI fixes have gone in :) |
|
Rebased, the branch is up-to-date again |
|
Thanks! |
The C17 standard is available since CMake 3.21. Since it pretty much just contains a couple of defect reports, it's probably save to allow it here.
At a quick glance I couldn't find a specific reason why C17 should not be allowed, but maybe there is something?
I'm not familiar with the code base, so I'm not sure if it would be OK to also allow C23, as that version actually removes some features.
Summary by CodeRabbit
New Features
Chores