Skip to content

ENH: Select standard-dependent header code from ITK_CXX_ABI - #7118

Closed
hjmjohnson wants to merge 3 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-itk-cxx-abi-level
Closed

hjmjohnson wants to merge 3 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-itk-cxx-abi-level

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

Implements #7116. itkConfigure.h records ITK_CXX_ABI (17, 20, 23, or 26), always the C++ standard ITK is built with, plus one ITK_CXX_ABI_STD_<FEATURE> macro per standard library feature ITK found and uses. ITK headers select standard-dependent code from those macros and no longer from __cplusplus or feature-test macros, so ITK and every client of one ITK build see the same definitions. This removes the One Definition Rule concern @N-Dekker raised on #7065.

Decisions taken (from #7116):

Changes
  • ITK_CXX_ABI is computed from CMAKE_CXX_STANDARD and forced on every configure; a value given on the command line is overwritten. ITKConfig.cmake exports it.
  • At level 20 or above, configure-time probes record ITK_CXX_ABI_STD_ENDIAN, ITK_CXX_ABI_STD_MATH_CONSTANTS, and ITK_CXX_ABI_STD_ATOMIC_REF. A client's own __cpp_lib_* macros are never consulted, so a client with a different compiler or standard library cannot select a different branch.
  • The probes run on every configure, so a changed standard, compiler, flag, or deployment target cannot leave a stale answer; itkConfigure.h is rewritten only if its content changes.
  • The client check in itkConfigure.h requires at least C++17 and at least the standard of ITK_CXX_ABI, and reads _MSVC_LANG when present.
  • All five existing sites are converted: ITK_UNEQUAL_OPERATOR_MEMBER_FUNCTION and ITK_NODISCARD in itkMacro.h test the level; itkByteSwapper.h, itkMath.h, and itkParallelSparseFieldLevelSetImageFilter.hxx test their recorded feature macro.
  • A pygrep pre-commit hook rejects a new #if on __cplusplus, _MSVC_LANG, or __cpp_* in Modules/**/*.h and *.hxx (outside ThirdParty). It reads one line at a time, so it does not follow backslash continuations.
  • Tests: itkConfigureGTest.cxx (value is a supported standard; feature macros only at level 20 or above; member operator!= follows the level), itkConfigureNewerClientTest (compiled with a newer standard than ITK, must still see ITK's definitions), and itkConfigureOlderClientTest (a client one standard below the level must be stopped by the error form of the check).
  • Separate commits add the migration guide entry and the agent guidance (AGENTS.md, Documentation/AI/compiler-cautions.md section 16).
Behavior changes to review
  • A C++20 client of a C++17 ITK build now gets the C++17 form of these headers (for example a member operator!=).
  • If ITK recorded a library feature and a client's standard library lacks it, the client fails to compile. The reverse case is consistent: both use the fallback.
  • MSVC without /Zc:__cplusplus reports __cplusplus as 199711L, so the two itkMacro.h conditions were false there even in a C++20 build. They now follow the configured standard. Not built on Windows locally.
  • The older-client warning now also covers a client below C++17 against a C++17 ITK; the previous check compared against a probe value that stopped at C++17.
  • ITK_COMPILED_CXX_STANDARD_VERSION is unchanged and no longer used by the check.
  • Not done: converting the guards in ENH: Select image iterator access by image constness (supersedes #7021) #7065.
Approaches tried and dropped
  • #if ITK_CXX_ABI >= 20 && defined(__cpp_lib_x): still reads the client's standard library.
  • A cached signature of standard, compiler, and flags to decide when to re-probe: it missed inputs (sysroot, per-configuration flags); probing on every configure is simpler.
  • Deleting itkConfigure.h when the inputs change: unnecessary, and it forced a full rebuild after unrelated flag changes.
  • if constexpr (ITK_CXX_ABI >= 20) as the main mechanism: it cannot select a class member, a base class, or an #include.
  • Including the generated itkConfigure.h directly from itkMath.h: the Doxygen warnings check could not resolve it.
Local verification (macOS arm64, Homebrew LLVM, C++17 build)
  • The three ITKCommon test drivers build; 98/98 related tests pass, including the three new tests. The newer client is compiled as C++20. The older-client test passes on repeated runs.
  • Negative control: with a __cplusplus test put back in itkMacro.h, itkConfigureNewerClientTest fails to compile.
  • A C++14 translation unit including itkConfigure.h gets the warning by default and the #error with ITK_FUTURE_LEGACY_REMOVE; a C++17 one gets neither.
  • A configure with CMAKE_CXX_STANDARD=20 -DITK_CXX_ABI=17 writes level 20 to itkConfigure.h, the cache, and ITKConfig.cmake, and defines all three feature macros. That tree was configured only, not built.
  • Mixed-standard probes (-fsyntax-only) of itkByteSwapper.h, itkMath.h, and an explicit instantiation of ParallelSparseFieldLevelSetImageFilter compile for C++17, C++20, and C++23 clients at level 17, and for C++20 and C++23 clients at level 20.
  • The hook regex rejects eight sample violations and accepts nine legitimate lines, including #ifdef __cplusplus; pre-commit run --all-files passes.
  • Not run here: a full ITK build configured as C++20 (the Azure Linux job does that), Windows and the Visual Studio generator, a client with a different standard library than ITK, and a downstream forest build. The level-set test driver is not in this build tree.
AI assistance

Claude Code (Fable 5.1) surveyed the existing sites, drafted the change and documentation, and ran the local checks. The author reviewed and is responsible for the change.

@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Segmentation Issues affecting the Segmentation module area:Documentation Issues affecting the Documentation module labels Nov 5, 2026
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 7a0dba9 -> 1206e82 is content only (no rebase): itkMath.h no longer includes the generated itkConfigure.h directly, which the Doxygen warnings check could not resolve; it gets ITK_CXX_ABI through itkMathDetail.h.

@hjmjohnson
hjmjohnson force-pushed the enh-itk-cxx-abi-level branch 2 times, most recently from 1206e82 to 854f96d Compare November 5, 2026 15:00
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 1206e82 -> 854f96d is content only (no rebase).

  • Library features are now recorded at configure time as ITK_CXX_ABI_STD_ENDIAN, ITK_CXX_ABI_STD_MATH_CONSTANTS, and ITK_CXX_ABI_STD_ATOMIC_REF. The earlier form, ITK_CXX_ABI >= 20 && defined(__cpp_lib_x), still read the client's standard library, so a client with a different toolchain than the ITK binaries could select a different branch.
  • The client check also rejects a client below C++17; at the default level it previously checked nothing.
  • The pre-commit hook no longer exempts lines that mention ITK_CXX_ABI, and matches #elifdef and (__cplusplus) forms.

@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 854f96d -> 62ca6bc is content only (no rebase). The single commit is split into three: code and pre-commit hook, migration guide, and agent documentation. No C++ or CMake line changed.

Also in this push: the hook no longer flags #if defined(__cplusplus) followed by a comment, #ifdef __cplusplus_cli, or a __cpp_ name inside a trailing comment; the agent guide states that #ifdef __cplusplus is allowed; the migration guide names the C++17 floor.

@hjmjohnson
hjmjohnson force-pushed the enh-itk-cxx-abi-level branch 2 times, most recently from 62ca6bc to 00d0a33 Compare November 5, 2026 15:14
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 62ca6bc -> 00d0a33 is content only (no rebase): when CMAKE_CXX_STANDARD, the compiler, or CMAKE_CXX_FLAGS changes in an existing build tree, CMake now clears the cached feature probes and deletes and regenerates itkConfigure.h. Only Modules/Core/Common/CMakeLists.txt changed.

@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 00d0a33 -> 1fe1473 is content only (no rebase): the configure signature also covers CMAKE_CXX_EXTENSIONS, CMAKE_OSX_DEPLOYMENT_TARGET, and CMAKE_OSX_SYSROOT, since the deployment target decides whether Apple's library provides std::atomic_ref.

@hjmjohnson
hjmjohnson force-pushed the enh-itk-cxx-abi-level branch 2 times, most recently from 1fe1473 to 3e7cb9e Compare November 5, 2026 15:48
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 1fe1473 -> 3e7cb9e is content only (no rebase): the explicit delete of itkConfigure.h is removed. Clearing the cached probes is sufficient, and configure_file rewrites the header only when its content changes, which avoids a full rebuild after an unrelated flag change.

@hjmjohnson hjmjohnson changed the title WIP: ENH: Select standard-dependent header code from ITK_CXX_ABI ENH: Select standard-dependent header code from ITK_CXX_ABI Nov 5, 2026
@hjmjohnson
hjmjohnson marked this pull request as ready for review November 5, 2026 16:08
@greptile-apps

This comment was marked as resolved.

Comment thread Modules/Core/Common/CMakeLists.txt Resolved
@hjmjohnson
hjmjohnson force-pushed the enh-itk-cxx-abi-level branch from 3e7cb9e to 33ef2c0 Compare November 5, 2026 16:26
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 3e7cb9e -> 33ef2c0 is content only (no rebase): addresses both Greptile findings. The feature probes run on every configure in place of a cached signature, and two tests cover a newer-standard client and an older-standard client.

@github-actions github-actions Bot added the type:Enhancement Improvement of existing methods or implementation label Nov 5, 2026
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 33ef2c0 -> 3fb223a is content only (no rebase), test CMake only: the older-client test is skipped when cross-compiling and passes --config only for multi-config generators, and the newer-client test also tries C++26.

@hjmjohnson
hjmjohnson force-pushed the enh-itk-cxx-abi-level branch from 33ef2c0 to 3fb223a Compare November 5, 2026 16:30
@blowekamp

Copy link
Copy Markdown
Member

I am not a big fan of doing a try compile for each conditionally used std C++ header. Configuration can be atime consuming step.

@hjmjohnson
hjmjohnson force-pushed the enh-itk-cxx-abi-level branch from 3fb223a to 0544669 Compare November 5, 2026 16:46
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 3fb223a -> 0544669 is content only (no rebase). It applies the decisions now recorded in #7116:

  • ITK_CXX_ABI always equals the build standard and cannot be set separately; ITKConfig.cmake exports it as the floor for downstream projects.
  • A client on an older standard gets a warning in this release (an #error with ITK_FUTURE_LEGACY_REMOVE), in place of the unconditional #error.
  • itkConfigureOlderClientTest exercises the error form, so it recompiles on every run.

ITK supports clients built with a newer C++ standard, or another
compiler, than ITK. Headers that test __cplusplus or a feature-test
macro then give ITK's libraries and the client different definitions of
one class or inline function, which violates the One Definition Rule.

itkConfigure.h now records ITK_CXX_ABI, always the standard ITK is
built with, and one ITK_CXX_ABI_STD_<FEATURE> macro per library feature
found at configure time; the headers test only those. The level is the
floor for clients: an older client gets a warning in this release.
Clients built with an older standard than ITK now get a warning (an
error with ITK_FUTURE_LEGACY_REMOVE), and downstream headers that
select code by C++ standard should test the configured level.
Agents reach for __cplusplus and feature-test macros by default; the
guide states the rule and the reason so new headers follow it.
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 0544669 -> f330df3 is content only (no rebase): the migration-guide commit message now says an older client gets a warning, and the older-client test defines ITK_FUTURE_LEGACY_REMOVE as empty so it matches the header when ITK itself is configured with it.

@hjmjohnson

hjmjohnson commented Nov 5, 2026 •

Copy link
Copy Markdown
Member Author

Closing in favor of #7119. This PR adds too much complexity for very little benefit: a level macro, three feature macros, configure-time probes, an exported CMake variable, and three tests, all to keep C++20 code paths that only a C++20 build of ITK would use.

#7119 solves the same One Definition Rule problem by having ITK headers always present their C++17 form, with the newer forms kept behind an unsupported, off-by-default ITK_ALPHA_FEATURE_CXX20 switch. It needs no probes, which also answers @blowekamp's concern about configure time above.

@hjmjohnson hjmjohnson closed this Nov 5, 2026
hjmjohnson added a commit that referenced this pull request Nov 5, 2026
ENH: Present the C++17 form of ITK headers in every C++ standard (supersedes #7118)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:Documentation Issues affecting the Documentation module area:Segmentation Issues affecting the Segmentation module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants