Repository navigation
Conversation
Each concept is now a std::bool_constant computed with standard type traits or the detection idiom, and itkConceptMacro expands to a static_assert naming the failed check. This replaces the Boost-derived Constraints/Enforcer machinery, its compiler-specific variants, and the ITK_CONCEPT_NO_CHECKING switch. HasNumericTraits now also rejects types that only have the primary NumericTraits template.
ITK_USE_CONCEPT_CHECKING is no longer defined, so checks in modules ingested from remote repositories never compiled. Enable them, fix the MovingImageDimension definition they exposed in PhaseCorrelationImageRegistrationMethod, and add checks to RieszFrequencyFilterBankGenerator and MedialThicknessImageFilter3D. Remove a check on a fixed type and empty placeholder blocks.
Only checks expressible without itk::Concept are converted: dimension comparisons, SameType, integer and signedness, and constructibility.
1919d32 to
b20af2c
Compare
|
Thank you for improving this change Niels! |
[Medium impact] The PR changes compile-time type checks across many ITK modules. Fix the complex-image build failure and remove the unused include to meet the project standard before merging. Findings
|
| // Begin concept checking | ||
| itkConceptMacro(Input1ConvertableToOutputCheck, (Concept::Convertible<Input1PixelType, OutputPixelType>)); | ||
| itkConceptMacro(Input2ConvertableToOutputCheck, (Concept::Convertible<Input2PixelType, OutputPixelType>)); | ||
| itkConceptMacro(Input1GreaterThanInput2Check, (Concept::GreaterThanComparable<Input1PixelType, Input2PixelType>)); |
There was a problem hiding this comment.
MaximumAbsoluteValueImageFilter<Image<std::complex<float>, 3>> now fails to compile because this enabled check requires > and >= on complex pixels. The filter compares their magnitudes through Math::Absolute, which returns a floating-point value, then returns the selected complex pixel. Check the magnitude types instead, or remove the raw-pixel ordering check.
| itkConceptMacro(Input1GreaterThanInput2Check, (Concept::GreaterThanComparable<Input1PixelType, Input2PixelType>)); |
| }; | ||
|
|
||
| /** Concept requiring T to have a trait called ValueType */ | ||
| struct HasPixelTraits : std::bool_constant<Detail::IsValidExpression<Detail::PixelTraitsMembers, T>> |
There was a problem hiding this comment.
HasPixelTraits<std::complex<double>>::value produces a compile error instead of false. Looking up PixelTraits<T>::ValueType creates the primary PixelTraits class, whose own TPixelType::ValueType declaration fails outside the scope that IsValidExpression can catch. Callers therefore cannot use this boolean trait in if constexpr to choose a fallback. Make the primary PixelTraits safe to probe and add a false-case test.
| template <typename TVoid, template <typename...> typename TExpression, typename... TArgs> | ||
| struct IsValidExpressionImpl : std::false_type | ||
| {}; | ||
| template <bool> | ||
| struct UniqueType_bool | ||
| template <template <typename...> typename TExpression, typename... TArgs> | ||
| struct IsValidExpressionImpl<std::void_t<TExpression<TArgs...>>, TExpression, TArgs...> : std::true_type | ||
| {}; | ||
| /** True when the type alias TExpression<TArgs...> is well-formed (detection idiom). */ | ||
| template <template <typename...> typename TExpression, typename... TArgs> | ||
| inline constexpr bool IsValidExpression = IsValidExpressionImpl<void, TExpression, TArgs...>::value; |
There was a problem hiding this comment.
The rewritten traits have no committed tests for false results. The remaining checks in itkNumericTraitsTest.cxx all require success, and the invalid std::vector<float> check is deleted rather than changed to assert false. Commit the local pass/fail cases as positive and negative static_assert tests, including rejected HasNumericTraits and Convertible cases. Otherwise CI cannot catch a check that silently starts accepting invalid types.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| #include "itkNumericTraits.h" | ||
| #include <iostream> | ||
| #include <type_traits> | ||
| #include <utility> |
There was a problem hiding this comment.
Unused include violates project standards
The added <utility> include has no user. Lvalue replaces std::declval, and nothing else here uses that header. The project standard says “Include only what you use.” Remove the include before merging to satisfy that requirement.
| #include <utility> |
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reimplement
itk::Conceptasstd::bool_constanttraits checked bystatic_assert, enable concept checks that the removedITK_USE_CONCEPT_CHECKINGhad silently disabled, and use plainstatic_assertwhere no concept is needed.Commits
itkConceptChecking.hgoes from 981 to 277 lines. Concepts use standard type traits or the detection idiom, anditkConceptMacrois astatic_assertwhose message names the check and the concept.MovingImageDimensioninPhaseCorrelationImageRegistrationMethod, which was derived from the fixed image type.ImageDimension == OutputImageDimensionandstd::is_same_v. The remaining 757 uses keepitkConceptMacro.Behavior changes
HasNumericTraitsrejects types that have only the primaryNumericTraitstemplate. It caughtitkNumericTraitsTestassertingHasNumericTraits<std::vector<float>>without includingitkNumericTraitsStdVector.h; that specialization lacksIsPositiveand the related functions, so the assertion is removed.ITK_CONCEPT_NO_CHECKINGand theitkConceptConstraintsMacroandConcept::Detailhelpers for custom concepts. The only external user found is dmritool'sutlITKConceptChecking.h.Local testing (macOS arm64, Apple clang 21)
MacOS-Rel-Pythondashboard configuration (ITK_LEGACY_REMOVE=ON, ITKReview, Montage, RLEImage, …) plus the ingested modules: 3 failures, all in IsotropicWavelets'itk::Matrix<std::complex<T>>andVariableSizeMatrix<std::complex<T>>wrappers.GetInversereachesitkBridgeMathSVD.handitkBridgeEigenDecompositionSignConvention.h, which compare complex values with<and>. This reproduces on unmodifiedupstream/main, and this PR touches neither file.ctest: 6590 of 6623 pass, including all C++ tests. Of the 33 Python failures, 32 come from_IsotropicWaveletsPythonnot building (see above). The other isPythonMedianImageFilterTest, a Python 3.15typingerror inget_type_hints.AI assistance
🤖 Generated with Claude Code