Skip to content

BUG: Fix iterator copy and PathIterator::Value() bugs, and test iterator access - #7135

Draft
hjmjohnson wants to merge 5 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-iterator-coverage-tests
Draft

hjmjohnson wants to merge 5 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-iterator-coverage-tests

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

Fixes two image iterator bugs on main and adds tests for the iterators' current write access, conversions, and copies. This splits the fixes and the coverage that does not depend on the rework out of #7065, which will be rebased on this PR.

Requires #7068 to merge first: its two commits come first on this branch; review from "BUG: Keep the position and reflection state in reflective iterator copies".

  • BUG: Copying a ReflectiveImageRegionConstIterator rewound it, and operator= dropped m_IsFirstPass.
  • BUG: PathIterator::Value() did not compile: it called a GetImage() member and read an m_ImageIndex member that do not exist.
  • ENH: Tests for Set() and Value() access, one-way conversion from writable to read-only iterators and their comparison, deduction from raw image pointers, reverse traversal of a sub-region, and copies that outlive their source.
Local verification
Left for #7065

These checks do not hold on main and stay with the rework: ImageReverseConstIterator and ImageRegionReverseConstIterator provide Set() through a const_cast; LineConstIterator, PathConstIterator, and ReflectiveImageRegionConstIterator have no deduction guide; only ImageIteratorWithIndex has ImageIteratorDimension.

RandomPermutation::operator= reallocated its node array but never copied
the nodes into it, so every node kept index zero. Copy-assigning an
ImageRandomNonRepeatingConstIteratorWithIndex therefore produced an
iterator that visited the region's first pixel for every sample.

Copy the nodes through a new copy constructor, which also gives the copy
its own generator, seeded with the source's seed: the implicit one shared
the node array, and sharing the generator let reseeding one permutation
change the other's order. The iterator's operator= now copies the sample
counters too, so a target that was default-constructed or over another
region visits the source's samples. SetPriority rejects an index equal
to the size.
…erator

The implicit copy constructor of ImageRandomNonRepeatingConstIteratorWithIndex
copied the m_Permutation pointer, so the copy and the source shared one
permutation and each deleted it. A copy used after the source was destroyed
read freed memory. The copy constructor now copies the permutation and the
sample counters, as operator= does.
…pies

The copy constructor of ReflectiveImageRegionConstIterator rewound the
copy to the beginning, and operator= did not copy m_IsFirstPass, so a
copy did not continue where the source was. Both now copy the full
state; code that relied on a copy starting over calls GoToBegin() on it.
Value() called a GetImage() member and read an m_ImageIndex member that
PathIterator does not have, so any call to it failed to compile. It now
returns the pixel at m_CurrentImageIndex, as Set() writes it.
@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances 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:Filtering Issues affecting the Filtering module labels Nov 6, 2026
Covers, for the current iterator names: which iterators provide Set()
and an assignable Value(), one-way conversion from writable to read-only
iterators and comparison between them, class template argument
deduction from raw image pointers, reverse traversal of a sub-region,
reflective iterator copies, and random non-repeating conversions that
outlive their source.
@hjmjohnson
hjmjohnson force-pushed the enh-iterator-coverage-tests branch from 8a2fce5 to 9229cb6 Compare November 8, 2026 03:10
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push 8a2fce5e10d..9229cb6c578 changes content only, with the same base. It fixes the Pixi-Cxx windows-2022 failure: MSVC error C2039 in itkFloodFillIteratorGTest, where the HasSet checks paired the ImageFunction flood-fill iterators with SphereSpatialFunction, which has no EvaluateAtIndex().

What changed (one commit: "ENH: Test image iterator write access, conversions, and copies")
  • The ImageFunction and ShapedImageFunction flood-fill iterators in OnlyWritableIteratorsSet now use BinaryThresholdImageFunction; the spatial iterator keeps SphereSpatialFunction. MSVC instantiates the virtual IsPixelIncluded() with the class, which GCC and Clang do not.
  • ITKCommon declares ITKImageFunction in TEST_DEPENDS for that header, with the usual "introduced by" comment; it was reachable only through ITKImageGrid's COMPILE_DEPENDS.
  • The test's ITK includes are grouped ahead of the standard headers.

#7065's first five commits are these five again (git range-diff: all identical).

Local verification at 9229cb6
  • ninja ITKCommonGTestDriver ITKDistanceMapGTestDriver (macOS arm64, Release): no compiler warnings.
  • ctest -R 'FloodFillIterator|Reflective|ImageIterators': 22/22 passed.
  • pre-commit run --all-files passes.
  • The MSVC error cannot be reproduced on macOS; Windows CI is the check for it.

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:Filtering Issues affecting the Filtering module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances 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.

1 participant