Skip to content

Require C++17 for public headers in CMakeLists#6627

Open
traversaro wants to merge 2 commits into
assimp:masterfrom
traversaro:patch-2
Open

Require C++17 for public headers in CMakeLists#6627
traversaro wants to merge 2 commits into
assimp:masterfrom
traversaro:patch-2

Conversation

@traversaro
Copy link
Copy Markdown
Contributor

@traversaro traversaro commented May 1, 2026 •
edited by coderabbitai Bot
Loading

Since #6592 that got released in assimp 6.0.5, inline constexpr is used in the public headers, that requires the downstream compilation units to be compiled with at least C++ standard 17 .

Setting TARGET_COMPILE_FEATURES(assimp PUBLIC cxx_std_17) ensures that any downstream compilation unit that links assimp::assimp is aware of this, and raise if possible and necessary the C++ standard used for compilation to C++17, avoiding errors like:

[1/2] Building CXX object CMakeFiles\assimp_main.dir\assimp_main.cpp.obj
FAILED: [code=2] CMakeFiles/assimp_main.dir/assimp_main.cpp.obj 
C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1444~1.352\bin\Hostx64\x64\cl.exe  /nologo /TP  -external:I%PREFIX%\Library\include -external:W0 /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MD /O2 /Ob2 /DNDEBUG /showIncludes /FoCMakeFiles\assimp_main.dir\assimp_main.cpp.obj /FdCMakeFiles\assimp_main.dir\ /FS -c %SRC_DIR%\test\assimp_main.cpp
%PREFIX%\Library\include\assimp/defs.h(302): error C7525: inline variables require at least '/std:c++17'
ninja: build stopped: subcommand failed.

Summary by CodeRabbit

  • Chores
    • Updated the build system to publicly declare C++17 as a minimum language requirement for the assimp library.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 1, 2026 •
edited
Loading

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 399e7769-b8e4-4c4e-a494-09a4f10521fb

📥 Commits

Reviewing files that changed from the base of the PR and between 392a658 and 60a42b5.

📒 Files selected for processing (1)
  • code/CMakeLists.txt
📝 Walkthrough

Walkthrough

The build configuration for the assimp CMake target now declares a public C++17 language requirement via TARGET_COMPILE_FEATURES. Previously, only private C standard requirements were configured. Consumers will inherit this C++17 minimum.

Changes

Cohort / File(s) Summary
Build Configuration
code/CMakeLists.txt		 Added public C++17 language feature requirement to assimp target, propagating the minimum C++ standard to all consumers of the library.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A hop and a bound through the compile-time ground,
Now C++17 standards are publicly found!
The assimp target declares with a flourish so bright,
Its consumers inherit the language requirement right. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and directly describes the main change: adding a C++17 requirement for public headers in CMakeLists.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@traversaro traversaro mentioned this pull request May 1, 2026
Merged
3 tasks
kimkulling
kimkulling approved these changes May 12, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@kimkulling kimkulling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine for me.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 12, 2026

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kimkulling kimkulling kimkulling approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@traversaro @kimkulling