Skip to content

ENH: Add itkVirtual/itkFinal/itkNonVirtual macro variants#6253

Open
hjmjohnson wants to merge 2 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:virtual-override-final-macros
Open

ENH: Add itkVirtual/itkFinal/itkNonVirtual macro variants#6253
hjmjohnson wants to merge 2 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:virtual-override-final-macros

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson commented May 11, 2026

Add itkVirtual/itkFinal/itkNonVirtual variants for all itkSet*/itkGet* macros. Legacy macros unchanged — zero behavioral/ABI change. Silences 611 -Wunnecessary-virtual-specifier warnings in ANTs (issue #6232, PR #2785).

Backward-compatibility guarantee

100 % backward-compatible. Every legacy macro name continues to expand to the virtual variant — byte-identical generated code, identical vtable layout, identical ABI. No deprecation gate, no ITK_FUTURE_LEGACY_REMOVE change, no symbol churn, no API removed/renamed/relocated. Downstream code that never types itkVirtual* / itkFinal* / itkNonVirtual* sees no behavioral or compilation difference. The PR only adds new optional spellings; no existing class is migrated.

What this PR does

For each member-function macro family in itkMacro.h:

  1. An internal *Impl helper parametrized by (virtualKeyword, finalKeyword) — the DRY shared body.
  2. Three public variants:
    • itkVirtual*Macrovirtual (matches today's behavior)
    • itkFinal*Macrofinal (seals further overriding)
    • itkNonVirtual*Macro → no virtual-specifier (new methods in final leaf classes)
  3. The legacy spelling, redefined to expand to the itkVirtual*Macro variant.

No itkOverride* variants are provided: re-declaring an inherited Set/Get in a subclass with the exact same macro-generated body just duplicates the base-class implementation. If an override is genuinely needed (different behavior, extra validation, side effects), write the function by hand. The pre-existing itkOverrideGetNameOfClassMacro is unchanged.

Minimal non-breaking foundation for PR #2785 and issue #6232.

Why this matters — concrete ANTs evidence

ANTs has shipped final leaf classes (command-line option types, filter wrappers, statistical containers, image-curvature analyzers, etc.) since at least 2019. For that entire period the legacy macros offered no mechanism to silence Clang's -Wunnecessary-virtual-specifier on the resulting virtual accessors — the only workaround was abandoning the macros and losing itkDebugMacro / Modified() / float-equal-suppression standardization.

Clean ANTs master build against this PR:

  • Baseline (legacy macros): 626 warnings across 14 final-marked headers.
  • After mechanically substituting itkNonVirtual* at 167 call sites in those 14 headers: 15 remaining (residue is hand-written virtual keywords, not macro-generated).
  • Net: 611 warnings silenced — 97.6 % — by call-site spelling alone. No change to function bodies, signatures, or behavior.
Design: the DRY pattern
// One body, three variants, one legacy alias — per family.

#define itkSetMacroImpl(virtualKeyword, finalKeyword, name, type) \
  virtualKeyword void Set##name(type _arg) finalKeyword           \
  {                                                               \
    itkDebugMacro("setting " #name " to " << _arg);               \
    /* ...identical body to original itkSetMacro... */            \
  }                                                               \
  ITK_MACROEND_NOOP_STATEMENT

#define itkVirtualSetMacro(name, type)    itkSetMacroImpl(virtual, , name, type)
#define itkFinalSetMacro(name, type)      itkSetMacroImpl(, final, name, type)
#define itkNonVirtualSetMacro(name, type) itkSetMacroImpl(, , name, type)
#define itkSetMacro(name, type)           itkSetMacroImpl(virtual, , name, type)  // legacy

At most one of virtualKeyword and finalKeyword is non-empty; itkNonVirtual* passes both empty.

Local verification
  • pre-commit run --all-files passes at the pushed HEAD.
  • ITKCommon and module header tests build clean.
  • ANTs master build against this PR yields the 626→15 warning delta above.

@github-actions github-actions Bot added type:Enhancement Improvement of existing methods or implementation area:Core Issues affecting the Core module labels May 11, 2026
@hjmjohnson hjmjohnson force-pushed the virtual-override-final-macros branch 3 times, most recently from 74eef42 to beacd3c Compare May 11, 2026 20:55
Comment thread Modules/Core/Common/include/itkMacro.h Outdated
@hjmjohnson
Copy link
Copy Markdown
Member Author

@greptile review

@greptile-apps

This comment was marked as resolved.

Comment thread Modules/Core/Common/include/itkMacro.h Outdated
@hjmjohnson hjmjohnson force-pushed the virtual-override-final-macros branch from beacd3c to ea5329c Compare May 12, 2026 15:33
@hjmjohnson hjmjohnson changed the title ENH: Add itkVirtual/itkOverride/itkFinal/itkNonVirtual macro variants ENH: Add itkVirtual/itkFinal/itkNonVirtual macro variants May 12, 2026
@hjmjohnson hjmjohnson force-pushed the virtual-override-final-macros branch from ea5329c to 016b685 Compare May 12, 2026 15:46
@hjmjohnson hjmjohnson marked this pull request as ready for review May 12, 2026 15:47
@hjmjohnson hjmjohnson requested a review from N-Dekker May 12, 2026 16:07
Comment thread Modules/Core/Common/include/itkMacro.h Outdated
hjmjohnson and others added 2 commits May 13, 2026 15:37
Bump the .gitattributes hooks-max-size attribute for
Modules/Core/Common/include/itkMacro.h from the 100000-byte default
to 200000 bytes ahead of upcoming work that adds the
itkVirtual/itkFinal/itkNonVirtual macro variants. After those
families are added the header grows from ~90 KB to ~110 KB,
exceeding the default ghostflow/kw-pre-commit size guard.

Matches the precedent set for itkPatchBasedDenoisingImageFilter.hxx
(120000 bytes).
Introduce explicit virtual/final/non-virtual specifier variants for
the ITK Set/Get macro families:

  * itkSetMacro, itkGetMacro, itkGetConstMacro,
    itkGetConstReferenceMacro, itkBooleanMacro
  * itkSet/Get/SetGetInput, itkSet/Get/SetGetDecoratedInput,
    itkSet/Get/SetGetDecoratedObjectInput,
    itkSet/GetDecoratedOutput
  * itkSet/GetEnum, itkSet/GetString, itkSetClamp, itkSetObject,
    itkGet/GetConst/GetModifiable/GetConstReferenceObject,
    itkSetConstObject, itkSet/GetVector

Each family now expands a single internal ITK_DETAIL_*MacroImpl
helper parametrized by virtual-specifier and trailing
virt-specifier. The three new variants itkVirtual*, itkFinal*,
itkNonVirtual* expand to that helper with the appropriate
specifiers; the legacy itk*Macro form expands to the itkVirtual*
variant. The Object family preserves the ITK_FUTURE_LEGACY_REMOVE
ifdef branch. No behavioral or ABI change.

Internal helpers use the ITK_DETAIL_<X>MacroImpl namespace
(uppercase ITK_DETAIL_ prefix mirrors ITK's existing
"namespace detail" convention for implementation helpers). Each
helper carries a "Do not invoke ... directly from user code"
comment above its definition.

No itkOverride* variants are provided: re-declaring an inherited
Set/Get with an identical macro-generated body duplicates the
base. Write the function by hand if a real override is needed.

Co-Authored-By: Niels Dekker <N.Dekker@lumc.nl>
@hjmjohnson hjmjohnson force-pushed the virtual-override-final-macros branch from 016b685 to 2766eb5 Compare May 13, 2026 21:28
@hjmjohnson hjmjohnson requested a review from N-Dekker May 13, 2026 22:27
@hjmjohnson hjmjohnson self-assigned this May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Enhancement Improvement of existing methods or implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants