ENH: Master tracking — virtual / override / final semantics on Get/Set macros for ITK 6.x

[hjmjohnson]

ENH: Master tracking — virtual / override / final semantics on Get/Set macros for ITK 6.x

Why this issue

Two long-stalled PRs (#2785 by @N-Dekker and #2568 by @jhlegarreta) and a 2018 Discourse thread on the adjacent NeighborhoodIterator question have established a pattern: the design conversation about whether ITK Get/Set macros should emit virtual lives only in scattered PR threads and has no GitHub Issue umbrella. As a result, individual incremental PRs keep getting filed without consensus on direction, and reviewers (especially @blowekamp) keep asking for a top-level discussion before more patches land.

This issue is a single venue to converge on what we want Get/Set polymorphism to look like in ITK 6 and 7, what is grandfathered, and what concrete sub-PRs the consolidated direction implies. ITKv6 is permitted to break or revise historical promises where the cost-benefit warrants it; the goal here is to make those revisions intentional, not accidental.

Historical baseline (load-bearing context)

Several decisions and observations were made deliberately and are documented across Discourse + Gerrit + GitHub:

1. Removal of virtual from ConstNeighborhoodIterator is the empirical precedent. Discourse #814 (2018-03) is the canonical thread:

   • @N-Dekker: HoughTransform2DCirclesImageFilter went from > 10 minutes → < 30 seconds (40× speedup, ITK 4.13 vs ITK master with virtual removed from ConstNeighborhoodIterator).
   • @phcerdan independently reproduced an 18–24% reduction in HoughTransform run-time after the removal.
   • @blowekamp tried broad removal across all iterators and got 56 test failures including 15 SEGFAULTs (most caused by vtable-dependent code in ShapedNeighborhoodIterator).
   • @phcerdan: "the destructor of the bases should be virtual at least" — i.e., not all virtual is equal; removing only non-destructor virtual is the safe direction.
   • @blowekamp ended up using CRTP (Curiously Recurring Template Pattern) for the Shaped family — "a bit overkill, so if more elegant solutions suggested please propose."

2. final-as-stress-test is a known technique (@N-Dekker, Discourse #814, post #4): "I first declared all of them 'final', as a WIP, just to be sure that they were not overridden anywhere in ITK." The same technique was used in the 2026-05-07 audit for Get/Set macros (see "Empirical override surface" below).

3. Performance motivation is established for non-iterator hot paths too. Every virtual Get/Set call in a hot loop forces vtable indirection and prevents inlining — exactly the same compiler-optimization story as the iterator case.

The two stalled PRs

#2785 — @N-Dekker, 2021-09-12: "WIP: Remove `virtual` from Get/Set macro definitions"

• 21 files modified across Modules/.
• WIP because of @N-Dekker's own 2024-11 hesitation: "I find this a difficult issue. … Maybe also final if we want to ensure no derived class overrides."
• Engagement: most recent activity Nov 2024.
• No Fixes: linkage; no associated issue.
• Effective design proposal: make non-virtual the default in the macros. ABI-breaking for any out-of-tree derived class that currently overrides; no breakage for callers.

#2568 — @jhlegarreta, 2020-06-25: "STYLE: Use macros to Set/Get ivars in `Common` operators"

• 11 files modified, all Modules/Core/Common.
• Stalled because it is upstream of WIP: Remove virtual from Get/Set macro definitions #2785: every hand-rolled Get/Set converted to the macro becomes a place where the virtual-vs-non-virtual default matters. @jhlegarreta is blocked on knowing what the macro will emit.
• Engagement: stale. Last comment from @jhlegarreta cites WIP: Remove virtual from Get/Set macro definitions #2785.

The unmerged additive work (2026-05-07 session)

Branch hjmjohnson:macro-optionally-virtual (commit ffc70ed0b9, +40 lines, single file) adds:

• A complete family of itkVirtual…Macro wrappers (28 aliases) — itkVirtualSetMacro, itkVirtualGetMacro, itkVirtualGetConstMacro, … — that delegate to the existing macros today. Marker macros at base-class sites. Zero behavior change, additive only.
• The ITK_GETSET_VIRTUAL tripwire alongside ITK_ITERATOR_VIRTUAL — empty in ITK 6, static_assert(false, ...) under ITK_FUTURE_LEGACY_REMOVE to flag override sites that need attention.

Not opened as a PR yet pending the discussion in this issue.

Empirical override surface (2026-05-07 final-keyword audit)

Same final-as-stress-test technique @N-Dekker pioneered in 2018 was applied to the current Get/Set macros (every macro-emitted method declared final in a sandbox build):

• 17 unique base methods are overridden by ≥ 1 derived class in upstream ITK.
• 19 unique source files affected; 1305 total compile errors (template-instantiation amplification).
• Top breakage sites: itkImageAdaptor.hxx (825 errors — single root cause: ImageBase::Get{Spacing,Origin,Direction}), itkImageToImageMetricv4.h (172), itkPointSetToPointSetMetricWithIndexv4.h (78), itkStatisticsImageFilter.h (63).

This validates @N-Dekker's 2024 hesitation empirically: removing virtual blindly would break ~17 distinct override sites. Conversely, most macros could go non-virtual safely with no downstream impact if the 17 known sites are migrated to explicit itkVirtualSetMacro first.

Questions that need answers

Q1. Should the Get/Set macros emit virtual by default?

• Status quo: they do (e.g. itkSetMacro produces virtual void SetX(...)).
• Option A (incremental): keep status quo; add itkVirtualSetMacro etc. as explicit-virtual variants for use at base-class sites that genuinely need polymorphism. New code uses non-virtual macros where appropriate; existing code unchanged. (My PR-A direction.)
• Option B (default flip, ITK 6): make the default macros emit non-virtual. Add itkVirtualSetMacro for the 17 known override sites. ABI break for downstream code that overrides macro-emitted methods (rare; the audit identifies the upstream cases).
• Option C (default flip, ITK 7): Option A in ITK 6 with ITK_FUTURE_LEGACY_REMOVE tripwire (ITK_GETSET_VIRTUAL); flip default in ITK 7 after a downstream-dashboard cycle confirms no out-of-tree breakage.

Q2. Should the destructor of base classes emit virtual?

@phcerdan's 2018 caveat: at least the destructor must remain virtual on polymorphic base classes. The Get/Set macros have nothing to do with destructors, but Q1 + #2785 must not accidentally remove virtual from a base destructor. Confirm the macros do not emit destructors.

Q3. CRTP for legitimately-polymorphic Get/Set sites?

Some base methods should be polymorphic (e.g. ImageAdaptor::GetSpacing proxies through the underlying image). For those:

• Keep virtual. Use itkVirtualGetConstMacro so the intent is visible at the call site.
• Or migrate to CRTP (per @blowekamp's 2018 NeighborhoodIterator pattern). CRTP is invasive; only worth the cost for genuine hot paths.

Decision needed per-site.

Q4. ITK_FUTURE_LEGACY_REMOVE tripwire — yes or no?

The ITK_GETSET_VIRTUAL marker (in PR-A) is the equivalent of ITK_ITERATOR_VIRTUAL. Empty in ITK 6, static_assert under ITK_FUTURE_LEGACY_REMOVE. This lets downstream consumers run the legacy-remove check on their dashboards before the default flip. Recommend adopting; needs maintainer ack.

Q5. Coordinate or supersede #2785 and #2568?

@N-Dekker is still engaged on #2785 (Nov 2024). @jhlegarreta is blocked on #2785 in #2568. Two paths:

• Coordinate: post on WIP: Remove virtual from Get/Set macro definitions #2785 + STYLE: Use macros to Set/Get ivars in Common operators #2568 pointing at this issue, ask @N-Dekker if PR-A's additive itkVirtualSetMacro approach is acceptable as the ITK 6 step. If yes, WIP: Remove virtual from Get/Set macro definitions #2785 is reframed as "convert 17 known sites to itkVirtualSetMacro" instead of "remove virtual everywhere."
• Supersede: open a fresh PR with the consolidated direction; close WIP: Remove virtual from Get/Set macro definitions #2785/STYLE: Use macros to Set/Get ivars in Common operators #2568 with credit. Faster but burns goodwill if @N-Dekker is willing to drive WIP: Remove virtual from Get/Set macro definitions #2785 after the design is settled.

Strong preference: coordinate.

Q6. Performance evidence for ITK 6?

@phcerdan's 2018 18-24% on HoughTransform was for ConstNeighborhoodIterator. Get/Set macros are not in equivalent hot paths, but the 17 known override sites include ImageBase::Get{Spacing,Origin,Direction} which IS hot. A small benchmark (e.g. PerformanceBenchmarking module) measuring registration / metrics with and without virtual on those three methods would justify or kill the urgency.

Q7. ITKv6 license to break

ITK 6 may revise historical promises. If the cost of ABI breakage on macro-emitted overrides is low (the 17 sites are all in-tree), Option B becomes viable. If maintainers have evidence of out-of-tree consumers that override macro-emitted methods, that constrains us toward Option C.

Maintainer positions summarized (best understanding)

Maintainer	Position
@N-Dekker	Original virtual-removal champion (NeighborhoodIterator 2018, Get/Set macros 2021); pioneered final-as-stress-test technique; 2024 hesitation: "this is a difficult issue"
@blowekamp	Tried broad iterator removal in 2018, hit 56 test failures + 15 SEGFAULTs; introduced CRTP fallback for Shaped family; wants design discussion before more patches
@phcerdan	Reproduced 18-24% perf gain in 2018; warned about base-class destructor virtuality
@jhlegarreta	Blocked on #2785 (Q1 answer); willing reviewer when direction is settled
@dzenanz	Permissive reviewer; would benefit from Q1 quorum to unblock incremental PRs
@hjmjohnson	Filed this issue; authored PR-A macro-optionally-virtual; ran 2026-05-07 final-keyword audit

Concrete asks

1. Each maintainer leaves a comment with their Q1 preference (A / B / C) and any non-obvious Q2–Q7 position.
2. Once Q1 has a quorum:
   • Option A → land PR-A as a no-op, then convert 17 known sites to itkVirtualSetMacro in a follow-up. WIP: Remove virtual from Get/Set macro definitions #2785 reframes; STYLE: Use macros to Set/Get ivars in Common operators #2568 unblocks.
   • Option B → coordinate ABI-break announcement in ITK 6 release notes; WIP: Remove virtual from Get/Set macro definitions #2785 lands as default-flip.
   • Option C → land PR-A + tripwire in ITK 6; flip default in ITK 7 after legacy-remove dashboard cycle.
3. If Q3 surfaces hot-path Get/Set sites that warrant CRTP, open separate per-site PRs; do not bundle with the macro question.

[dzenanz]

It is hard to make up my mind. This refactoring has potential to create a lot of fallout, without equally significant benefit. So maybe option A: have both virtual and not-virtual macros, and for now (and maybe permanently) the regular Get/Set macros delegate to virtual variants.

[hjmjohnson]

@dzenanz I think that having both virtual and not-virtual macros is the right option. We need the not-virtual options for final classes (ANTs has a lot of final classes, but ITK does not.). If ITK starts making final classes to lock down the API, then the not-virtual methods become very important.