ENH: Select image iterator access by image constness (supersedes #7021) by hjmjohnson · Pull Request #7065 · InsightSoftwareConsortium/ITK

hjmjohnson · 2026-11-01T15:09:09Z

Closes #6141 and fixes #7003: the image iterators now take their access from the constness of the image type, so XxxIterator<const ImageType> has no Set() and no writable Value(), and none of them needs const_cast. Supersedes #7021 (its commits are here, authored by @mymonin) and #7117.

Built on #7135, which is built on #7068; both must merge first. Their five commits come first here, unchanged; review from "STYLE: Remove the writable ImageIterator subclass" onward. #7135 carries the bug fixes and the tests that hold on main; this PR extends those tests to iterators over const images.

Commit layout for review. Most commits need no careful reading:

Per iterator family, three STYLE: commits that do not build alone: delete the writable subclass files, .hxx included (pure deletion), git mv the Const files onto the writable names (renamed without changes), and rename the class identifiers (the body gives the perl command that reproduces it).
Two trailing STYLE: commits (= default copy operations, stale comments), the ENH: Test … commits, and the guide.

The commits that change behavior, each small and compiling on its own:

Commit	What to check
ENH: Refactor ImageIterator/ImageConstIterator	the design: SFINAE `Set()`/`Value()`, `m_Buffer` constness, the derived `ImageConstIterator`
ENH: Restrict ImageIterator conversions by constness	`detail::IsImageIteratorConvertible_v`
ENH: Select … access by image constness (7 commits)	the same pattern applied to one family each
ENH: Select reverse iterator access by image constness	also moves the row spans and `operator++`/`--` into `ImageReverseIterator`
ENH: Write flood-filled iterator pixels without const_cast	unchanged from the earlier revision

Design

The writable name carries the implementation: ImageRegionIterator<TImage> derives from ImageIterator<TImage>, and pixel pointers are const exactly when TImage is const, so writes need no cast.
Set() and non-const Value() are removed for const TImage with SFINAE.
ImageRegionConstIterator<T> is a thin class deriving from ImageRegionIterator<const T>, with deduction guides for SmartPointer, ConstPointer, and raw pointers. Each header provides both names; the Const headers remain as one-line includes, and the .hxx files of removed classes are deleted, since only their headers included them.
detail::IsImageIteratorConvertible_v states the allowed conversions: same image type, never dropping const. Comparisons require the same image type, with or without const.
ImageReverseIterator<T> carries the row spans and operator++/-- (on main it had them through ImageRegionReverseConstIterator<T>), so ImageReverseConstIterator<T> gains them; ImageRegionReverseIterator<T> derives from it and adds only constructors.
One form in every C++ standard, following ENH: Present the C++17 form of ITK headers in every C++ standard (supersedes #7118) #7119.
The flood-filled iterators keep the writable image pointer their constructor receives.
Not converted here, as follow-ups: the neighborhood iterators, SliceIterator, and the RLEImage iterator specializations.

API changes

Listed in the ITK 6 migration guide (last commit). Removed, by design:

Writing through an iterator over a const image (const-qualified images can be modified via itk::ImageRegionIterator<const ImageType> #7003), including the public ImageReverseConstIterator::Set().
Converting a read-only iterator into a writable one.
The XxxIterator<T> to XxxConstIterator<T> base-class relation: Superclass changes, and a non-const XxxConstIterator<T> & no longer accepts an XxxIterator<T>.

Also: the frequency layout iterators initialize m_ActualXDimensionIsOdd. The reflective copy, random non-repeating copy, and PathIterator::Value() fixes are in #7135. Kept from ITK 5: ImageIteratorDimension and construction of the exclusion and frequency iterators from a plain ImageIteratorWithIndex.

Testing

Known issue at this head: "ENH: Select ImageIteratorWithIndex access by image constness" fails BUG: Fix iterator copy and PathIterator::Value() bugs, and test iterator access #7135's ExclusionIteratorConstructibleFromImageIteratorWithIndex check; the next ENH commit builds. Every other commit expected to build compiles the five iterator test sources with no warnings. The refactor commit now carries the same-image comparison constraint, so it passes BUG: Fix iterator copy and PathIterator::Value() bugs, and test iterator access #7135's HaveConstNonConstComparisons check.
Compile-time HasSet checks now cover every converted iterator, including the frequency, reflective, PathIterator, and flood-filled ones.
Not yet run: the downstream forest build (ENH: Modernize image iterators to eliminate const_cast via VIsConst templating #6141 acceptance criterion 5).
Full build at the tip (macOS arm64, conda clang, Release, C++17, Module_RLEImage ON); ITKCommon, ITKDistanceMap, ITKImageFrequency, ITKPath, and RLEImage tests: 872/872 pass. The only warnings are the existing ones in the IsotropicWavelets and RANSAC remote modules.
Doxygen on every header this PR changes reports no warnings.
pre-commit run --all-files passes at the tip, git diff --check passes on every commit, and every rename commit equals its perl rule applied to its parent.

AI assistance (Claude Code) rebuilt the commit series with scripts kept for review and ran the checks; the author reviewed the changes.

# Conflicts: # Modules/Core/Common/include/itkImageRandomConstIteratorWithIndex.h # Modules/Core/Common/include/itkImageRandomConstIteratorWithIndex.hxx

hjmjohnson · 2026-11-01T23:36:40Z

@greptileai please review the latest commits (6dca3f7).

# Conflicts: # Modules/Core/Common/include/itkImageRandomConstIteratorWithIndex.h # Modules/Core/Common/include/itkImageRandomConstIteratorWithIndex.hxx

hjmjohnson · 2026-11-02T04:07:32Z

@greptileai please review the latest commits (1e416e2).

hjmjohnson · 2026-11-02T08:28:41Z

@greptileai please review the latest commits (3f3d071).

# Conflicts: # Modules/Core/Common/include/itkImageRandomConstIteratorWithIndex.h # Modules/Core/Common/include/itkImageRandomConstIteratorWithIndex.hxx

hjmjohnson · 2026-11-02T15:36:01Z

Ran this PR at 3f3d0711140 through the downstream forest testbed: no new failures. ITK was built with every module and Python wrapping, and 19 downstream projects were rebuilt and tested against it. Every failure was traced to a cause outside these changes, and wherever a failure could involve iterator code, the same build against main gave the same result.

What was built

ITK main at fc6366677db, with this PR at 3f3d0711140 (the current head, including the review fixes) merged together with nine other open PRs (#7020, #7041, #7046, #7048, #7051, #7052, #7056, #7060, #7064) as one integration branch. Every consumer was rebuilt against that ITK, and every tested binary was checked to be newer than the start of the run. Slicer builds its own ITK, so it got a copy of the same branch.

This PR and #7041 overlap in itkImageRandom*IteratorWithIndex; the integration branch moves #7041's detail::RandomIndexInRegion helper into the new location.

Results

Project	Build	Tests
ITK, all modules, Python wrapping	pass	6837/6849, see below
ANTs	pass	185/185
BRAINSTools	pass	462/462
RTK	pass	92/92
Impact	pass	51/51
VkFFTBackend	pass	24/24
TractographyTRX	pass	9/9
PerformanceBenchmarking	pass	6/6
Cleaver	pass	4/4
OpenIGTLink	pass	63/63
SphinxExamples	pass	508/508 non-interactive
elastix	pass *	160/164, identical to `main`
SimpleITK	pass	554/558, see below
SimpleITKFilters	pass	2/8, see below
Slicer, with its own ITK built from the same branch	pass	660/676, see below
Slicer extensions (AnomalousFilters, BoneTexture, SlicerElastix, SlicerIGSIO)	pass	not run
IGSIO	pass	19/30, see below
OpenIGTLinkIO, vtkAddon, PlusLib	pass	0/6, 1/6, 2/32, see below

* With three VNL includes added, without which elastix does not compile against ITK main. That fix is SuperElastix/elastix#1489.

How each failure was traced

Compared against a build of main at 1e281cd110e without these PRs, with identical results:

ITK DeformableRegistration8 with a grid of 100 (ITKv3...Test11, Test12, DeformableRegistration8Test3): killed after about 35 s. The same program built against main is killed the same way, at about 11 GB resident.
elastix (4 tests): built against main without these PRs, it fails the same tests with the same numbers.
SimpleITK CoherenceEnhancingDiffusion: with SimpleITK's default parameters, the filter's output is bit-identical against main and against this branch.
SimpleITK DICOM tests (3): with Module_ITKIODCMTK on, DCMTKImageIO wins the ImageIO factory and reads int16 data as uint8; a build of main picks the same IO. ConcurrentImageRead.DICOM crashes the same way against main.
Slicer CastScalarVolume unsigned tests (3): the cast output is identical against main. The baselines encode x86 wrap-around for negative floats cast to unsigned types, which is undefined behavior; arm64 saturates to 0.
Slicer SegmentationsModuleTest2: the ITK margin step (signed Maurer distance plus binary threshold) gives identical output against main; the difference comes from Slicer's labelmap merge.

Outside the code this PR changes:

ITK Python tests (9):
- Modules ITK's own CI does not build: IOFDF, PhaseSymmetry, MeshNoise, BoneMorphometry, and a second ParabolicMorphology that collides with ITK's copy.
- Wrapping gaps in RANSAC and Strain; missing MinimalPathExtraction test data; a Python 3.14 typing error; an assertion in itkImageMetaDataSetGetItem.py.
SimpleITKFilters (6): four tests lack their input image, which ExternalData did not fetch; the KWStyle and GoogleTest drivers are not built.
Slicer, the other 12: GUI and module-widget tests, SSL, the extensions manager, MergeModels, a vtkAddon math test, and standalone Python import. None was traced to ITK.
SlicerANTs and SlicerRT: both fail at CMake configure. In ANTs it is export set "antsTargets" not found; in Plastimatch it is ZLIB::ZLIB not found through ITK::ITKZLIBModule.
IGSIO, OpenIGTLinkIO, vtkAddon, PlusLib: the failure sets match the previous two sweeps, which used different ITK commits; the crashes trace to two VTK builds loaded in one process. One IGSIO test that crashed in this run, VolumeReconstructorTestRunNearMeanUChar, passed six times out of six when rerun alone.

SimonRit

I did not do a thorough review, too many changes... But LGTM.

N-Dekker · 2026-11-04T15:24:04Z

I did not do a thorough review, too many changes

Thanks Simon. I just hope that others will do a thorough review, because the iterator framework is really a fundamental part of ITK. However, I also find +6.449 -7.246 lines of code changes too much to review carefully. 🤷

SimonRit · 2026-11-04T15:34:36Z

I did not do a thorough review, too many changes

Thanks Simon. I just hope that others will do a thorough review, because the iterator framework is really a fundamental part of ITK. However, I also find +6.449 -7.246 lines of code changes too much to review carefully. 🤷

Me too! It's not only the number of changes, large parts of the code have been moved without changes which makes the reviewing more difficult. The good news is that it's thoroughly tested as it is a fundamental part of ITK :-).

git mv of each itkXxxConst* file onto the itkXxx* name; the content is unchanged. The read-only code becomes the implementation of both names, so its history follows it. This commit does not build alone.

Mechanical: in each renamed file, with C its read-only class and W the writable one, perl -pi -e 'next if /^\s*\*\s*\\(sa|sphinxexample)\b/; s/\bitkC(_h|_hxx|\.h|\.hxx)\b/itkW$1/g; s/\bC\b/W/g' FILE Other classes named in the file keep their names here. \sa and \sphinxexample lines keep the read-only names. This commit does not build alone.

hjmjohnson · 2026-11-07T04:14:50Z

Force-push f44a52b3a40..994865e0f98 changes content only, with the same base. It moves the same-image constraint on the six comparison operators from "ENH: Restrict ImageIterator conversions…" into "ENH: Refactor ImageIterator/ImageConstIterator", so that commit now passes #7135's HaveConstNonConstComparisons check. The tip tree is identical; the Restrict commit is retitled to cover conversions only.

Checks

git range-diff: only those two commits differ; every other commit's tree is unchanged.
The refactor commit compiles the five iterator test sources with no warnings.
pre-commit run --all-files passes.

ImageReverseConstIterator offered a public Set() that cast away const, and the writable reverse iterators accepted read-only sources. ImageReverseIterator<T> and ImageRegionReverseIterator<T> now write only when T is not const, and convert only toward const; ImageReverseConstIterator<T> and ImageRegionReverseConstIterator<T> are the iterators over const T. The row spans and operator++/-- move from the region class into ImageReverseIterator, which had them through its base, so ImageReverseConstIterator gains them. The GetBufferPointer test now converts from a writable iterator.

Each XxxIterator<T> here derived from XxxConstIterator<T> and added Set() and Value() by casting away const. The next commits make the read-only class the implementation, with access selected by the constness of T, so the subclass and its out-of-line definitions go away whole; only the headers included the .hxx files. Removing the files first lets the following rename show as a pure rename. This commit does not build alone.

git mv of each itkXxxConst* file onto the itkXxx* name; the content is unchanged. The read-only code becomes the implementation of both names, so its history follows it. This commit does not build alone.

Mechanical, reproducible in the include directory with perl -pi -e 'next if /^\s*\*\s*\\(sa|sphinxexample)\b/; s/\bitkImageConstIteratorWithIndex(_h|_hxx|\.h|\.hxx)\b/itkImageIteratorWithIndex$1/g; s/\bImageConstIteratorWithIndex\b/ImageIteratorWithIndex/g' itkImageIteratorWithIndex.h itkImageIteratorWithIndex.hxx \sa and \sphinxexample lines keep the read-only names. This commit does not build alone.

Apply the ImageIterator design to the root of the WithIndex family: ImageIteratorWithIndex<T> holds the position as a pointer to const pixels only when T is const, so Set() and Value() need no const_cast and are absent for const images. ImageConstIteratorWithIndex<T> is ImageIteratorWithIndex<const T>.

Each XxxIterator<T> here derived from XxxConstIterator<T> and added Set() and Value() by casting away const. The next commits make the read-only class the implementation, with access selected by the constness of T, so the subclasses and their out-of-line definitions go away whole; only the headers included the .hxx files. Removing the files first lets the following rename show as a pure rename. This commit does not build alone.

git mv of each itkXxxConst* file onto the itkXxx* name; the content is unchanged. The read-only code becomes the implementation of both names, so its history follows it. This commit does not build alone.

Mechanical: in each renamed file, with C its read-only class and W the writable one, perl -pi -e 'next if /^\s*\*\s*\\(sa|sphinxexample)\b/; s/\bitkC(_h|_hxx|\.h|\.hxx)\b/itkW$1/g; s/\bC\b/W/g' FILE Other classes named in the file keep their names here. \sa and \sphinxexample lines keep the read-only names. This commit does not build alone.

Apply the ImageIteratorWithIndex design to the region, linear, slice, random, random non-repeating, and region exclusion iterators. Each XxxIteratorWithIndex<T> derives from its writable base, inherits the constness-gated Set() and Value(), and XxxConstIteratorWithIndex<T> is XxxIteratorWithIndex<const T>. The random non-repeating conversion deep-copies the permutation it owns.

Each XxxIterator<T> here derived from XxxConstIterator<T> and added Set() and Value() by casting away const. The next commits make the read-only class the implementation, with access selected by the constness of T, so the subclasses and their out-of-line definitions go away whole; only the headers included the .hxx files. Removing the files first lets the following rename show as a pure rename. This commit does not build alone.

git mv of each itkXxxConst* file onto the itkXxx* name; the content is unchanged. The read-only code becomes the implementation of both names, so its history follows it. This commit does not build alone.

Mechanical: in each renamed file, with C its read-only class and W the writable one, perl -pi -e 'next if /^\s*\*\s*\\(sa|sphinxexample)\b/; s/\bitkC(_h|_hxx|\.h|\.hxx)\b/itkW$1/g; s/\bC\b/W/g' FILE Other classes named in the file keep their names here. \sa and \sphinxexample lines keep the read-only names. This commit does not build alone.

The frequency-layout iterators and ReflectiveImageRegionIterator inherited from read-only bases and cast away const to write. They now derive from the writable WithIndex iterators, so writes need no cast and are unavailable for const images; each Const name is the iterator over the const image.

Each XxxIterator<T> here derived from XxxConstIterator<T> and added Set() and Value() by casting away const. The next commits make the read-only class the implementation, with access selected by the constness of T, so the subclasses and their out-of-line definitions go away whole; only the headers included the .hxx files. Removing the files first lets the following rename show as a pure rename. This commit does not build alone.

git mv of each itkXxxConst* file onto the itkXxx* name; the content is unchanged. The read-only code becomes the implementation of both names, so its history follows it. This commit does not build alone.

Mechanical: in each renamed file, with C its read-only class and W the writable one, perl -pi -e 'next if /^\s*\*\s*\\(sa|sphinxexample)\b/; s/\bitkC(_h|_hxx|\.h|\.hxx)\b/itkW$1/g; s/\bC\b/W/g' FILE Other classes named in the file keep their names here. \sa and \sphinxexample lines keep the read-only names. This commit does not build alone.

LineIterator and PathIterator wrote through a const_cast of the const image held by their read-only bases. Each is now the iterator, writable only when TImage is not const, and the Const name iterates over the const image. PathIterator::Value() read a nonexistent member, so it could not compile; it now returns the current pixel.

The writable flood-filled iterators cast away the const image held by ConditionalConstIterator to write a pixel. They now keep the writable image pointer their constructor receives, offer Set() only for non-const images, and read through the const image without a cast.

The hand-written copy constructors and assignment operators copied every member and reset the accessor's begin pointer to the same buffer, which the defaults also do. Copying a default-constructed ImageIteratorWithIndex no longer dereferences its null image pointer.

These iterators have no superclass that already declares the type aliases, and m_Image is a WeakPointer, not a smart pointer.

Extends the iterator conversion tests to TIterator<const TImage>: it is reached from the writable iterator and converts to and from the read-only name, no writable iterator is built from or derives from a read-only one, and the iterators compare across these names. The indexed and exclusion iterators keep ImageIteratorDimension and their constructors from indexed iterators over const images.

Checks that Set() exists for a non-const image and not for a const image, for the image, reverse, indexed, line, path, reflective, frequency, and flood-filled iterators; that an assignable Value() follows the same rule for the image, reverse, and indexed iterators; and that the read-only reverse iterators no longer provide Set().

Checks class template argument deduction from raw pointers to const images, that the read-only reverse iterator walks a region backward, that a reflective iterator keeps its state when converted to an iterator over a const image, and that a random non-repeating iterator over a const image owns its permutation.

Explains which iterator names now select access by the constness of the image type, what no longer compiles, and how downstream code adapts.

hjmjohnson · 2026-11-07T04:33:25Z

Force-push 994865e0f98..f3d5cef7050 changes content only, with the same base. It addresses my Opus max review of 994865e0f98. These are doc, comment, and include fixes, each folded into the commit that introduced the issue; no behavior changes.

What changed

Reverse iterators: the class doc no longer says the class cannot move; the headers include itkImageIterator.h and itkImageReverseIterator.h and use their own IndexType/SizeType (852646c, stale-comments commit).
Frequency iterators: the docs name ImageRegionIteratorWithIndex as the base.
Stale comments: WeakPointer members are no longer called smart pointers; removed a restating copy-operations comment, a self-referencing \sa, and a duplicate \ingroup; completed a truncated operator++ doc.
"ENH: Test that iterators over a const image cannot write": the message now says the Value() checks cover the image, reverse, and indexed iterators only.
Migration guide: says that function templates must take the iterator type as a parameter, and that RLEImage users keep the Const names.

Still open: "ENH: Select ImageIteratorWithIndex access by image constness" fails #7135's exclusion-iterator constructibility check; a separate push will fix it. Not changed here: PathIterator::operator= skips m_IsAtEnd and m_ZeroOffset, which is already the case on main.

hjmjohnson mentioned this pull request Nov 1, 2026

ENH: Modernize image iterators to eliminate const_cast via VIsConst templating #6141

Open

This was referenced Nov 1, 2026

ENH: Eliminate const_cast from itk::ImageIterator #7021

Closed

const-qualified images can be modified via itk::ImageRegionIterator<const ImageType> #7003

Open

hjmjohnson requested review from N-Dekker and SimonRit November 1, 2026 16:52

hjmjohnson marked this pull request as ready for review November 1, 2026 16:53

This comment was marked as resolved.

Sign in to view

greptile-apps Bot reviewed Nov 1, 2026

View reviewed changes

This was referenced Nov 1, 2026

BUG: Copy the shuffled order when assigning a RandomPermutation #7068

Open

BUG: Keep reverse region iterators inside the region after SetIndex #7069

Merged

hjmjohnson force-pushed the enh-iterator-const-correctness branch from 614d0e2 to 6dca3f7 Compare November 1, 2026 19:49

greptile-apps Bot reviewed Nov 1, 2026

View reviewed changes

Comment thread Modules/Core/Common/include/itkImageRegionReverseIterator.h Resolved

hjmjohnson force-pushed the enh-iterator-const-correctness branch from 6dca3f7 to 1e416e2 Compare November 2, 2026 00:09

greptile-apps Bot reviewed Nov 2, 2026

View reviewed changes

hjmjohnson force-pushed the enh-iterator-const-correctness branch from 1e416e2 to 3f3d071 Compare November 2, 2026 04:31

SimonRit approved these changes Nov 4, 2026

View reviewed changes

hjmjohnson added 2 commits November 6, 2026 22:13

STYLE: Rename the reverse iterator Const files

719b0bd

git mv of each itkXxxConst* file onto the itkXxx* name; the content is unchanged. The read-only code becomes the implementation of both names, so its history follows it. This commit does not build alone.

hjmjohnson force-pushed the enh-iterator-const-correctness branch from f44a52b to 994865e Compare November 7, 2026 04:14

hjmjohnson added 24 commits November 6, 2026 22:24

STYLE: Rename the itkImageConstIteratorWithIndex files

64912e6

git mv of each itkXxxConst* file onto the itkXxx* name; the content is unchanged. The read-only code becomes the implementation of both names, so its history follows it. This commit does not build alone.

STYLE: Rename the WithIndex iterator Const files

5e134e4

git mv of each itkXxxConst* file onto the itkXxx* name; the content is unchanged. The read-only code becomes the implementation of both names, so its history follows it. This commit does not build alone.

STYLE: Rename the frequency and reflective iterator Const files

708a79d

git mv of each itkXxxConst* file onto the itkXxx* name; the content is unchanged. The read-only code becomes the implementation of both names, so its history follows it. This commit does not build alone.

STYLE: Rename the LineIterator and PathIterator Const files

851d2bb

git mv of each itkXxxConst* file onto the itkXxx* name; the content is unchanged. The read-only code becomes the implementation of both names, so its history follows it. This commit does not build alone.

STYLE: Remove stale comments from the image iterator headers

ed7fe77

These iterators have no superclass that already declares the type aliases, and m_Image is a WeakPointer, not a smart pointer.

DOC: Describe the image iterator constness change in the ITK 6 guide

f3d5cef

Explains which iterator names now select access by the constness of the image type, what no longer compiles, and how downstream code adapts.

hjmjohnson force-pushed the enh-iterator-const-correctness branch from 994865e to f3d5cef Compare November 7, 2026 04:33

Conversation

hjmjohnson commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hjmjohnson commented Nov 1, 2026

Uh oh!

Uh oh!

hjmjohnson commented Nov 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hjmjohnson commented Nov 2, 2026

Uh oh!

hjmjohnson commented Nov 2, 2026

Uh oh!

SimonRit left a comment

Choose a reason for hiding this comment

Uh oh!

N-Dekker commented Nov 4, 2026

Uh oh!

SimonRit commented Nov 4, 2026

Uh oh!

hjmjohnson commented Nov 7, 2026

Uh oh!

hjmjohnson commented Nov 7, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hjmjohnson commented Nov 1, 2026 •

edited

Loading