[agourakis82]

Make ImageRegionIterator<const TImage> read-only so a const-qualified image cannot be modified through Set or Value. Addresses #7003. The other mutable iterator classes mentioned in that issue remain outside this patch.

Root cause and fix

ImageRegionIterator exposes Set and mutable Value even when its TImage template argument is const. Those methods can still reach mutable image storage, allowing a ConstPointer to be used to modify a supposedly read-only image. Constrain the two write accessors to non-const image types while preserving construction, Get, and the inherited read-only Value() access for const-qualified iterators.

AI assistance

AI assistance contributed to root-cause analysis, implementation, and regression tests. The code and validation evidence were reviewed before opening this draft; the contributor remains responsible for the submitted change.

Validation

A native C++23 reproducer built against the ITK Common SDK demonstrated that Set through ImageRegionIterator<const ImageType> changes a value held by a ConstPointer. A baseline build of itkImageIteratorsGTest.cxx failed the new compile-time checks for Set and mutable Value; with this change, the same GoogleTest target built and passed, including compile-time checks that the const-qualified iterator remains constructible and readable and that the ordinary mutable iterator retains both write APIs.

The Common source in the SDK matches the current upstream main checkout at base bd3e10fe79582057f59eb2fd6b3a40b60a6493f4. This is a focused native validation; full upstream CI is pending on this draft.

[hjmjohnson]

Thanks for the fix; the enable_if_t guard on Set and Value is the right shape. Please keep the BUG commit to the fix itself. Documentation and formatting changes should not ride along with a bug fix: if they are wanted, put them in a separate DOC: or STYLE: commit that follows ITK conventions. Please avoid unnecessary code churn.

Changes requested

Unrelated changes to drop from the BUG commit

• itkImageRegionIterator.h: the using typename Superclass::... lines were re-sorted. ITK's .clang-format sets SortUsingDeclarations: Never, so the existing order should stay.
• itkImageRegionIterator.h: the constructor comment and the \sphinxexample{...} line were rewrapped to about 80 columns. ITK's ColumnLimit is 120 and both lines were already within it.
• itkImageIteratorsGTest.cxx: several existing comments were rewrapped to 80 columns and blank lines between functions were removed. Please restore them.
• itkImageIteratorsGTest.cxx: the new test was inserted inside the comment of SupportClassTemplateArgumentDeduction, so the first half of that comment now sits above ConstQualifiedRegionIteratorIsReadOnly and the CTAD comment starts mid-sentence. Please place the new test after the CTAD test, or before its comment.

Running pre-commit run --all-files with ITK's own configuration should leave these lines untouched; an 80-column editor or formatter setting is the likely source.

Commit message

• Capitalize the word after the prefix and name the class, e.g. BUG: Make ImageRegionIterator<const TImage> Set and Value unavailable.
• Add a short body saying why: Set() and Value() could write through an image held by ConstPointer.

Scope

• #7003 notes that all non-const iterators share this hole. ImageIterator, ImageRegionIteratorWithIndex, ImageScanlineIterator, and the other non-const iterators still accept a const image and expose Set. Please change Closes #7003 to Addresses #7003, or extend the fix to those iterators.

Optional

• In the new test code, prefer std::is_const_v, std::remove_reference_t, and std::is_same_v, as used elsewhere in the file, and reuse the existing TestImageType alias.
• using Superclass::Value; would give ImageRegionIterator<const T> a read-only Value() const.

[agourakis82]

Thanks for the detailed review. The current diff restores the existing formatting and comments and places the regression after the CTAD test. It also exposes the inherited read-only Value() and uses the existing TestImageType alias and the _v traits.

I've changed the description to “Addresses #7003” and kept this patch scoped to ImageRegionIterator. I've also prepared a single BUG commit containing the current diff.

I'm keeping the PR in draft while investigating the macOS failure. The iterator tests passed, but itkMCI_DSC_case_2_binary aborted; that same test passed on the exact base commit. I haven't established the cause yet.

[agourakis82]

A clean paired build of the original ImageToVideoFilterTest passed for both base 3f19582 and PR merge 17185fa. Both used Apple Clang 17.0.0 under x86_64 Rosetta with C++17, keeping the original pixel assertion and fixtures unchanged. Both output images match the baseline byte for byte.

Paired diagnostic run (https://github.com/agourakis82/ITK/actions/runs/40669725689).

This does not explain the original full-build failure: cache reuse and the broader build configuration remain differences. Could a maintainer rerun only the failed ARMBUILD-x86_64-rosetta job?

[agourakis82]

Correction to the previous comment: the current head be4fc6e contains only the two intended files, leaves the existing declarations/comments unchanged, places ConstQualifiedRegionIteratorIsReadOnly after the CTAD test, reuses TestImageType and the existing type-trait style, and keeps Addresses #7003 with the other iterator classes out of scope.

The only current CI failure is ARMBUILD-x86_64-rosetta at ImageToVideoFilterTest in Modules/Video/Core/test/itkImageToVideoFilterTest.cxx. That file and the Video module are outside this PR's diff; the job reports no build warnings or errors. Please rerun only ARMBUILD-x86_64-rosetta on the current head and re-review the updated diff.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Summary

Makes ImageRegionIterator<const TImage> read-only while keeping ordinary mutable iterators writable.

• Const-qualified region iterators read pixels without exposing write access.

_{Reviews (1) · Last reviewed commit: "BUG: Make const-qualified ImageRegionIte..." · Reviewed by Greptile}

[Copilot]

🟢 Approval recommended

No active review issues remain for the change making const-image region iterators read-only while preserving mutable iterator behavior.

No open issues 🎉

What changed in this PR

Addresses #7003 by making ImageRegionIterator<const TImage> read-only while preserving read access and ordinary mutable iterator behavior. Other iterator classes remain outside this patch.

Changes:

• Restricts Set and mutable Value to non-const image types while exposing inherited read-only Value.
• Adds compile-time API checks and a runtime read-access regression test.

File	Description
Modules/​Core/​Common/​test/​itkImageIteratorsGTest.cxx	Verifies const-qualified read access and availability of write APIs only for mutable images.
Modules/​Core/​Common/​include/​itkImageRegionIterator.h	Constrains write accessors and preserves inherited read-only Value.

_{Review effort: Balanced}