BUG: Propagate test helper status and mark the helpers [[nodiscard]] - #6704
Merged
hjmjohnson merged 3 commits intoJul 27, 2026
Merged
Conversation
hjmjohnson
marked this pull request as ready for review
July 25, 2026 16:25
This comment was marked as resolved.
This comment was marked as resolved.
hjmjohnson
force-pushed
the
fix-propagate-helper-status
branch
from
July 25, 2026 16:55
b15c9d8 to
81a7a0e
Compare
Each of these tests dispatches to a templated helper selected by image dimension and discards the helper's return value, then returns EXIT_SUCCESS unconditionally. The tests therefore pass even when the helper reports failure, silencing 13 explicit failure returns and 31 ITK_TEST_* assertions. Capture the helper's status and return it.
The helper asserts a superclass of ImageToImageFilter, but the filter derives from ImageSink. MSVC evaluates the superclass check, so the helper returns EXIT_FAILURE on Windows.
Test sources define file-scope helpers returning EXIT_SUCCESS or EXIT_FAILURE, and a caller that drops the value turns every assertion inside the helper into a no-op. Marking the helpers [[nodiscard]] makes the compiler reject that. Annotate the 290 declarations of such helpers and propagate the status at the 30 call sites the attribute exposes, in eight tests spanning image functions, smoothing, diffusion, XML, metrics and superpixel segmentation. The attribute goes on every declaration, not just the definition: it only governs calls that follow the declaration carrying it, so a bare forward declaration leaves earlier call sites unchecked. Helpers whose int return is a value rather than a status are left unannotated, as are the few using a bare 0/1 convention.
hjmjohnson
force-pushed
the
fix-propagate-helper-status
branch
from
July 25, 2026 17:41
81a7a0e to
021999f
Compare
Member
Author
|
@greptileai review |
dzenanz
approved these changes
Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test helpers that return
EXIT_SUCCESS/EXIT_FAILUREare now[[nodiscard]], so a caller that drops the status no longer compiles. That plus the six original call-site fixes turns 13 explicit failure returns and 31ITK_TEST_*assertions back into real assertions, and exposed a wrong superclass assertion that only MSVC evaluates.The defect
The helpers do run — each one's argument-count guard matches its registered CMake invocation — so the defect is latent: the assertions execute but their verdict is discarded.
itkLabelOverlapMeasuresImageFilterTest.cxxreturn EXIT_FAILUREitkBSplineExponentialImageRegistrationTest.cxxITK_TEST_*itkBSplineImageRegistrationTest.cxxitkExponentialImageRegistrationTest.cxxitkTimeVaryingBSplineVelocityFieldImageRegistrationTest.cxxITK_TEST_*itkTimeVaryingVelocityFieldImageRegistrationTest.cxxITK_TEST_*Commit 2: the Windows failure this unmasked
itkLabelOverlapMeasuresImageFilterTest's helper assertsITK_EXERCISE_BASIC_OBJECT_METHODS(filter, LabelOverlapMeasuresImageFilter, ImageToImageFilter);but the filter derives from
ImageSink(itkLabelOverlapMeasuresImageFilter.h:43), and the enclosing test assertsImageSinkcorrectly 140 lines later.The assertion is compiler-dependent.
itkTestingMacros.h:54-83defines two variants: under__GNUC__the macro only prints the class name, while everywhere else it also comparesSuperclass::GetNameOfClass()and returnsEXIT_FAILUREon mismatch. GCC and Clang both define__GNUC__, so only MSVC evaluates the check — and only once commit 1 stopped discarding the helper's status did that reach the test result.Pixi-Cxx (windows-2022)was the sole red check before this fix.Commit 3: [[nodiscard]] across the test tree
290 declarations in 211 files. Selection is mechanical: a file-scope, non-entry-point function returning
intwhose body containsEXIT_SUCCESSorEXIT_FAILURE.The attribute goes on every declaration, not only the definition.
[[nodiscard]]governs calls that follow the declaration carrying it, so a bare forward declaration leaves earlier call sites unchecked — annotating the 14 forward declarations is what exposed theCoherenceEnhancingDiffusionTestsite below.Deliberately not annotated:
int-returning helpers whose return is a value, not a status (lookupinitkExceptionObjectTest, thedynamic_castDownCast*family, thetest*Splinehelpers). Annotating these would only force(void)casts at legitimate discard sites.TestKernelTransform, which uses a bare0/1convention and needs a semantics change first — handled separately.itkSyNImageRegistrationTestanditkBSplineSyNImageRegistrationTest, whose discarded call sites are fixed by BUG: Make the SyN registration tests actually run a registration #6703. Annotating them without that PR's argc fix makesitkBSplineSyNImageRegistrationTestfail. They can be annotated once BUG: Make the SyN registration tests actually run a registration #6703 lands.The attribute exposed 30 discarded call sites in 8 tests, all fixed here:
itkCentralDifferenceImageFunctionOnVectorSpeedTest.cxxitkObjectToObjectMultiMetricv4RegistrationTest.cxxitkDiscreteGaussianImageFilterTest2.cxxitkSLICImageFilterTest.cxxitkFFTDiscreteGaussianImageFilterTest.cxxitkFancyStringTest.cxxitkMeanSquaresImageToImageMetricv4OnVectorTest2.cxxCoherenceEnhancingDiffusionTest.cxxWhere a helper is called several times in one scope the status accumulates with
testStatus |=(the existing idiom initkColorTableTest.cxx); plain=is used only where the branches are mutually exclusive.Three of these were invisible to a source-level audit: in
itkFancyStringTestandCoherenceEnhancingDiffusionTestthe discard happens insideITK_TRY_EXPECT_NO_EXCEPTION(...), where the call is not lexically a statement. The compiler sees it after expansion; a grep does not.Test plan
Release, Ninja, AppleClang, macOS arm64, in a build tree configured against this branch alone (
Module_ITKReview=ONadded so no annotated file goes uncompiled).-Wunused-resultdiagnostics, zero errors, no new warnings.ctest -j8: 100% passed, 0 failed out of 6207.pre-commit run --all-filesexits 0.Not exercised locally: the annotated files under
Video/BridgeOpenCV,Video/BridgeVXLand the GPU modules, which need OpenCV / VXL / OpenCL. Every helper and call site in those 20 files was inspected by hand and all consume their return value. CI covers the compile.