ENH: Let Range classes do direct pixel access to itk::Image subclasses

[N-Dekker]

Until now, ImageBufferRange and ImageRegionRange would use a proxy to access the data of a subclass of itk::Image. With this commit, they will do direct pixel access instead, for those subclasses.

This commit may significantly improve the performance of iteration over the pixels of an itk::Image subclass. Moreover, it will address the compile errors, attempting to build OrfeoToolBox v10 against ITK 6.0 beta2, reported by Nicklas Larsson (@nilason) at #6898 saying:

itkCastImageFilter.hxx:167:84: error: non-const lvalue reference to type 'itk::Vector<double, 2>' cannot bind to an initializer list temporary
  167 |     std::conditional_t<isVariableLengthVector, OutputPixelType, OutputPixelType &> outputPixel{ *outputIt };
      |                                                                                    ^          ~~~~~~~~~~~~~

[N-Dekker]

• ImageSubclass originally taken from COMP: Fix cast filter proxy pixel writes (supersedes #6898) #6901 by @hjmjohnson

[greptile-apps]

Confidence Score: 4/5

[Medium risk] Refines pixel access detection for image subclasses.

Not ready to merge: the new trait comment and the previously reported subclass comments still violate a repository requirement.

Findings

1.  Remove redundant trait comment ▶
2.  Subclass pixel access lacks coverage ▶
3.  Remove redundant subclass comments ▶

Summary

The PR enables direct pixel access for itk::Image subclasses and adds a buffer-range test that reads and writes subclass pixels. The new trait comment violates the repository’s prose requirement and must be addressed before merging.

_{Reviews (2) · Last reviewed commit: "ENH: Let Range classes do direct pixel a..."}

[greptile-apps]

Remove redundant subclass comments

This comment describes the class and assertions immediately below it; the counterpart in the region-range test does the same. Both violate the repository directive against in-source explanations of code already visible in the diff. This repository requirement must be satisfied before merging.

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!

[greptile-apps]

Remove redundant trait comment

The new comment restates what IsDefaultPixelAccessorFunctor expresses and exceeds the repository’s 100-character limit for in-source comments. It violates the requirement for short, useful comments rather than explanations already visible in the code. This repository requirement must be satisfied before merging.

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!

[N-Dekker]

Sorry, I don't agree. I think the code should in general be human-readable without having to dive into commit history or discussions at GitHub. Where does the "100-character limit for in-source comments" come from? Such a limit does not make sense to me.

[greptile-apps]

You’re right that the comment can improve readability, and I overstated the rule as a general C++ style requirement. The 100-character limit comes from this PR’s AGENTS.md guidance, specifically Documentation/AI/prose-budget.md: it defines a one-line, ≤100-character budget for new in-source // comments. It is not a clang-format or general C++ requirement; .clang-format actually permits a 120-column limit.

That policy also says comments are appropriate when they preserve a non-obvious invariant. This comment explains the purpose of the trait, so retaining it is defensible; only its length exceeds that project-specific budget. A shorter version such as // Trait for the default pixel accessor functor. would preserve the explanation while complying. I’ll withdraw the request to remove the comment.

[N-Dekker]

@hjmjohnson Can we please 🙏 drop the "100-character limit for in-source comments"? I see, it was introduced with your pull request #6241. But I believe it's A Good Thing to add in-source comments to explain non-obvious internal implementation details. It is no problem to me if they sometimes exceed 100 characters. However, it is a problem if contributors get discouraged when they add useful comments.

I don't think the explanation of essential parts of the internal implementation should just be hidden somewhere in the commit history.

[dzenanz]

AGENTS.md is a guidance for agents. It is useful for Greptile sometimes reminding us of this. A lot of the time, it is wrong, and can (and should) be simply disregarded. Clicking "Resolve conversation" accomplishes that.

Perhaps we should remove the 100 character limit for agents, and replace it by single-line limit, effectively deferring to clang-format 120-column limit.

[greptile-apps]

I agree. A single-line requirement is a clearer and more practical constraint here: it keeps comments readable and compatible with the repository’s 120-column formatting convention, while avoiding an arbitrary 100-character cutoff that can discourage useful explanations. The guidance could say that new // comments should fit on one line and follow the project’s formatting limit, rather than imposing a separate 100-character budget. For this comment, the existing explanation is reasonable and should not be treated as a defect.

[dzenanz]

I proposed this change in #6906.

[greptile-apps]

Thanks — that addresses the concern. With the guidance change proposed in #6906, this comment is acceptable: it explains a non-obvious implementation detail and fits the project’s single-line formatting convention. I’m withdrawing the comment; no change is needed in this PR.

[N-Dekker]

Thanks but I still think the incentive is in the wrong direction. When proposing a useful comment, a guidance message from Greptile feels like a penalty, not a reward.

Anyway, do I understand correctly that contributors can avoid such AI generated guidance messages by using old-school C-style /* ... */ for comments that exceed 100 characters?

[greptile-apps]

No—not as a recommended or intended workaround. If a checker only matched newly added // comments, a /* ... */ comment might technically evade that particular check, but that would be a loophole rather than a meaningful project convention.

The guidance should depend on readability and purpose, not comment syntax. This useful explanatory comment should simply be accepted. Under the proposed guidance from #6906, the relevant constraint is a readable single line within the project’s formatting limit—not an arbitrary 100-character penalty. I agree that Greptile should not frame a useful comment as a defect merely because it exceeds that old budget.

[dzenanz]

Looks good on a glance.