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

# 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)](https://discourse.itk.org/t/removal-of-virtual-keywords-from-constneighborhooditerator/814) 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](https://discourse.itk.org/t/removal-of-virtual-keywords-from-constneighborhooditerator/814/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 #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 #2785.

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

Branch [`hjmjohnson:macro-optionally-virtual`](https://github.com/hjmjohnson/ITK/tree/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 #2785 + #2568 pointing at this issue, ask @N-Dekker if PR-A's additive `itkVirtualSetMacro` approach is acceptable as the ITK 6 step. If yes, #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 #2785/#2568 with credit. Faster but burns goodwill if @N-Dekker is willing to drive #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. #2785 reframes; #2568 unblocks.
- Option B → coordinate ABI-break announcement in ITK 6 release notes; #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.