Repository navigation
ENH: Present the C++17 form of ITK headers in every C++ standard (supersedes #7118) - #7119
Conversation
8f89cd9 to
1263b82
Compare
|
Force-push 8f89cd9 -> 1263b82 is content only (no rebase).
|
|
Is there any other, pre-existing muckery with compiler features that should be refactored into this style? |
|
@dzenanz Yes. I surveyed ITK's own headers, CMake probes, and generated headers for it, and filed the result as #7120 so this PR can stay small.
After this PR, the new pre-commit hook finds no unmarked test of |
1263b82 to
b96fd84
Compare
|
Force-push 1263b82 -> b96fd84 is content only (no rebase).
|
b96fd84 to
2818f95
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
Force-push 2818f95 -> d2c3ce3 is content only (no rebase): |
2818f95 to
d2c3ce3
Compare
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. The conditions that change a class member or an inline body now take their C++17 form unconditionally; newer forms stay behind ITK_ALPHA_FEATURE_CXX20, an unsupported advanced option recorded in itkConfigure.h (CXX23 and CXX26 are placeholders). Conditions that cannot change the ABI keep their test and are marked. A pre-commit hook rejects the rest. itkNewerStandardClientTest needs its own CXX_STANDARD, so it is a separate executable. Co-Authored-By: Niels Dekker <N.Dekker@lumc.nl> Co-Authored-By: Dženan Zukić <dzenan.zukic@kitware.com> Co-Authored-By: Bradley Lowekamp <blowekamp@mail.nih.gov>
Projects built with a newer standard than ITK now see ITK's C++17 definitions, and one experimental macro is no longer defined there.
Agents reach for __cplusplus and feature-test macros by default; the guide states the rule and the reason so new headers follow it.
d2c3ce3 to
c6a059b
Compare
dzenanz
left a comment
There was a problem hiding this comment.
Changes look good on a glance.
| // GOOD - write the C++17 form, with no condition. | ||
|
|
||
| // ACCEPTABLE - a newer form kept for the day ITK requires that standard: | ||
| #if defined(ITK_ALPHA_FEATURE_CXX20) |
There was a problem hiding this comment.
First of all, thanks @hjmjohnson ! Regarding the term ALPHA:
- It suggests to me that it is related to alpha releases of ITK. Is that was it is meant to be?
- It also suggests that there could be a "beta" follow-up, e.g., ITK_BETA_FEATURE_CXX20. Is that intended?
If both interpretations are wrong, I would rather use a different word, for example EXPERIMENTAL.
ITK headers now present one binary interface, the C++17 one, whatever C++ standard includes them, so ITK and every client see the same definitions. This removes the One Definition Rule concern @N-Dekker raised on #7065 without any configure-time machinery. Supersedes #7118, which added too much complexity for very little benefit. Addresses #7116; related cleanup is tracked in #7120.
Changes
ITK_UNEQUAL_OPERATOR_MEMBER_FUNCTIONinitkMacro.h(a class member, used in about 122 files) and thestd::atomic_refbranch initkParallelSparseFieldLevelSetImageFilter.hxx(bodies of template member functions).ITK_ALPHA_FEATURE_CXX20, an unsupported, testing-only opt-in that records which form to keep when ITK later requires C++20.ITK_ALPHA_FEATURE_CXX20,_CXX23, and_CXX26are advanced CMake options, off by default, declared in one loop and written toitkConfigure.hwith one#cmakedefineeach, so ITK and its clients cannot disagree. Turning one on with an olderCMAKE_CXX_STANDARDis a configure error. They are deliberately coarse: one switch per standard, found withgrep ITK_ALPHA_FEATURE_._CXX23and_CXX26are placeholders with no users yet.#ifline ends with// ABI-neutral: theITK_NODISCARD(message)attribute form initkMacro.h, the<numbers>include and bit-exactnessstatic_asserts initkMath.h, and thestd::endianinitializer initkByteSwapper.h(same value on either path).pygreppre-commit hook rejects an unmarked#ifon a__cplusplusvalue,_MSVC_LANG, or__cpp_*inModules/**/*.hand*.hxx(outside ThirdParty).#ifdef __cplusplusis allowed. It reads single lines only.itkNewerStandardClientTestis compiled with a newer standard than ITK and asserts it still sees the memberoperator!=. It is a separate executable because it needs its ownCXX_STANDARD. It includes onlyitkMacro.h, so it does not depend on the toolchain's C++20 library headers, and it turns off CMake's C++20 module scanning, which needs aclang-scan-depsthat some toolchains lack.AGENTS.md,Documentation/AI/compiler-cautions.mdsection 16).Behavior changes to review
operator!=on classes usingITK_UNEQUAL_OPERATOR_MEMBER_FUNCTION, andITK_EXPERIMENTAL_CXX20_REWRITTEN_UNEQUAL_OPERATORis no longer defined there.std::atomic_ref.itkMath.handitkByteSwapper.hbehave as before; their only change is the marker comment.#warninginitkConfigure.his unchanged; the only change to that file is the three#cmakedefinelines.Why not #7118
#7118 recorded the build standard as
ITK_CXX_ABIand probed three library features at configure time intoitkConfigure.h. It needed a level macro, three feature macros, threetry_compileprobes on every configure, an exported CMake variable, and three tests, to keep code paths that only a C++20 build of ITK would use. Downstream projects are on C++17, so those paths had almost no users. VTK takes the same position as this PR: all code is valid C++17, with no standard gating in headers.Local verification (macOS arm64, Homebrew LLVM)
ITKCommontest drivers and the new client test build; 37/37 related tests pass. The newer client is compiled as C++20. Configuring that tree withITK_ALPHA_FEATURE_CXX20=ONstops with the expected error.CMAKE_CXX_STANDARD=20,ITK_LEGACY_REMOVE=ON), switch off: the same targets build with no compiler warnings and 37/37 pass, with the newer client compiled as C++23. This is the check that keeping the memberoperator!=is clean in C++20.ITK_ALPHA_FEATURE_CXX20=ON:itkConfigure.hdefines the macro, the same targets build with no compiler warnings, and 37/37 pass. The client test then asserts the memberoperator!=is absent.ITK_UNEQUAL_OPERATOR_MEMBER_FUNCTION, with-Wambiguous-reversed-operator, was clean under clang at C++20 and C++23 and under GCC 16 at C++20.pre-commit run --all-filespasses, including the new hook, which finds no unmarked standard test in any ITK header.std::atomic_refalpha branch is not compiled.AI assistance
Claude Code (Fable 5.1) drafted the change and documentation, surveyed how other libraries handle the problem, and ran the local checks. The author reviewed and is responsible for the change.