build(engine): enforce C++17 in CMake and fix Data target standard#784
build(engine): enforce C++17 in CMake and fix Data target standard#784tianjianjiang wants to merge 1 commit intomasterfrom
Conversation
There was a problem hiding this comment.
The changes correctly enforce C++17 standard compliance across all build configurations. No critical issues found - the PR properly:
- Adds
CMAKE_CXX_STANDARD_REQUIRED ONto fail builds if C++17 is unavailable - Adds
CMAKE_CXX_EXTENSIONS OFFto disable compiler-specific extensions (e.g., GNU extensions) - Upgrades Data target from
gnu++0x(C++11 with GNU extensions) toc++17
This aligns with CLAUDE.md requirements and prevents accidental use of C++20/C++23 features.
There was a problem hiding this comment.
Code Review
This pull request aims to enforce strict C++17 compliance across the project. The changes correctly update the CMake build system to require C++17 and disable compiler extensions. The Xcode project's Data target is also updated from C++11 with GNU extensions to C++17.
My review identifies some redundant build settings in the subdirectory CMakeLists.txt files. These settings are already defined in the parent CMakeLists.txt and are inherited, so they can be removed from the subdirectory files to improve maintainability. I've added comments to the newly added redundant lines.
Overall, the changes are good and align with the goal of standardizing the C++ version.
There was a problem hiding this comment.
Pull request overview
This pull request enforces strict C++17 compliance across both the CMake-based C++ engine build and the Xcode project build system. Building on the guardrails established in #783, it eliminates outdated C++11 settings and disables GNU extensions to ensure consistent, standards-compliant C++ compilation.
Changes:
- Add
CMAKE_CXX_STANDARD_REQUIRED ONandCMAKE_CXX_EXTENSIONS OFFto all three CMakeLists.txt files for strict C++17 enforcement - Update Data target from obsolete
gnu++0x(C++11 with GNU extensions) toc++17in Xcode project
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Source/Engine/CMakeLists.txt | Add strict C++17 enforcement flags (STANDARD_REQUIRED ON, EXTENSIONS OFF) to main engine CMake configuration |
| Source/Engine/gramambular2/CMakeLists.txt | Add strict C++17 enforcement flags to gramambular2 library CMake configuration |
| Source/Engine/Mandarin/CMakeLists.txt | Add strict C++17 enforcement flags to Mandarin library CMake configuration |
| McBopomofo.xcodeproj/project.pbxproj | Update Data target from gnu++0x to c++17 in both Debug and Release configurations |
8a846ee to
fad05fd
Compare
dbae219 to
fe48e62
Compare
There was a problem hiding this comment.
The changes correctly enforce strict C++17 compliance across all CMake files and update the Data target from outdated C++11 with GNU extensions to C++17. The implementation is sound.
One note: verify that the Data target builds successfully with the stricter standard, as it previously used GNU extensions.
fe48e62 to
3e4114b
Compare
fad05fd to
972339b
Compare
3e4114b to
bd414b6
Compare
972339b to
218ed48
Compare
bd414b6 to
e1d7bb9
Compare
218ed48 to
988b086
Compare
e1d7bb9 to
a520883
Compare
988b086 to
34143ba
Compare
a520883 to
434ab66
Compare
- Add CMAKE_CXX_STANDARD_REQUIRED ON and CMAKE_CXX_EXTENSIONS OFF to all three CMakeLists.txt files (Engine, gramambular2, Mandarin) - Change Data target from gnu++0x (C++11) to c++17 in both Debug and Release configurations in project.pbxproj Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
34143ba to
bd56fc6
Compare
There was a problem hiding this comment.
Reviewed the diff and verified the project.pbxproj context. The two changed XCBuildConfiguration objects (6A38BC23 / 6A38BC24) belong to PBXLegacyTarget "Data" only — not project-wide — so the scope matches the PR description.
No critical issues found:
-
CMakeLists.txt (
Engine,gramambular2,Mandarin):CMAKE_CXX_STANDARD_REQUIRED ONandCMAKE_CXX_EXTENSIONS OFFare correct additions. The former turns a silent compiler downgrade into a hard error; the latter enforces portable ISO C++17 (i.e.,-std=c++17instead of-std=gnu++17). Both are safe on macOS/Clang, and all 100 engine tests continue to pass. -
project.pbxproj:gnu++0x(C++11 + GNU extensions) →c++17for the Data target is the right fix to align it with the rest of the project. Note thatDatais aPBXLegacyTargetthat runs a Makefile/shell script rather than a Clang compilation phase, soCLANG_CXX_LANGUAGE_STANDARDmay not be forwarded to the build tool as a compiler flag. The setting is harmless and documents intent, but the Makefile inSource/Data/is the authoritative place where any C++ compiler invocations would need to respect this standard.
Summary
CMAKE_CXX_STANDARD_REQUIRED ONandCMAKE_CXX_EXTENSIONS OFFto all three CMakeLists.txt files (Engine, gramambular2, Mandarin) to enforce strict C++17 compliancegnu++0x(C++11 with GNU extensions) toc++17in both Debug and Release configurations in project.pbxprojIndependent PR (base:
master). Previously part of the contextual user model stack; detached as it stands alone.Test plan
grep gnu++0x project.pbxprojreturns no matchesgrep STANDARD_REQUIREDreturns 3 hits (one per CMakeLists.txt)Generated with Claude Code