Skip to content

COMP: Move inline = default destructors to .cxx for 30 exported classes#6002

Merged
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:fix-exported-inline-destructors-abi
Apr 2, 2026
Merged

COMP: Move inline = default destructors to .cxx for 30 exported classes#6002
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:fix-exported-inline-destructors-abi

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Summary

This PR fixes an ABI issue in shared library builds (investigated and documented in issue #6000):

  • Problem: Non-template exported classes with ~ClassName() override = default; inline in the header have their destructor thunks (D1Ev/D0Ev) hidden under -fvisibility-inlines-hidden (an ITK global build flag), even when the class vtable and typeinfo remain exported via itkOverrideGetNameOfClassMacro. This causes dynamic_cast failures across DSO boundaries in shared library consumers.

  • Fix: Move the destructor definition to the corresponding .cxx file as ClassName::~ClassName() = default;. This compiles it in one TU with default (exported) visibility, correctly anchoring all destructor thunks.

  • Scope: 30 concrete non-template exported classes across 7 ITK modules (ITKCommon, ITKIOImageBase, ITKFEM, ITKOptimizers, ITKStatistics, ITKWatersheds, ITKVideoCore). Template classes were intentionally excluded — their destructors cannot be defined in .cxx files without explicit template instantiation boilerplate.

  • Verification: Full ITK build passes cleanly; all affected module tests pass.

Affected Classes (30)

Module Classes
ITKCommon EquivalencyTable, LoggerManager, LoggerOutput
ITKIOImageBase ArchetypeSeriesFileNames, NumericSeriesFileNames, RegularExpressionSeriesFileNames
ITKFEM FEMLightObject
ITKOptimizers ExhaustiveOptimizer, GradientDescentOptimizer, InitializationBiasedParticleSwarmOptimizer, MultipleValuedNonLinearOptimizer, OnePlusOneEvolutionaryOptimizer, Optimizer, QuaternionRigidTransformGradientDescentOptimizer, RegularStepGradientDescentBaseOptimizer, RegularStepGradientDescentOptimizer, SingleValuedNonLinearOptimizer, SPSAOptimizer, VersorTransformOptimizer
ITKStatistics ChiSquareDistribution, DenseFrequencyContainer2, GaussianDistribution, MaximumDecisionRule, MaximumRatioDecisionRule, MinimumDecisionRule, SparseFrequencyContainer2, TDistribution
ITKWatersheds OneWayEquivalencyTable, WatershedMiniPipelineProgressCommand
ITKVideoCore TemporalProcessObject

Related

@github-actions github-actions bot added type:Compiler Compiler support or related warnings area:Core Issues affecting the Core module area:IO Issues affecting the IO module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module area:Numerics Issues affecting the Numerics module labels Apr 1, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 1, 2026

Greptile Summary

This PR fixes a shared-library ABI visibility bug (issue #6000) across 30 exported ITK classes: under -fvisibility-inlines-hidden (an ITK-wide build flag), inline = default destructors in headers have their thunks (D1Ev/D0Ev) hidden, causing dynamic_cast failures across DSO boundaries even when the class vtable and typeinfo are exported. The fix — moving each destructor definition to its corresponding .cxx file — is the canonical solution for this class of problem and is applied uniformly and correctly across all 60 changed files.

  • All 30 class headers have the destructor changed from ~ClassName() override = default; (inline) to a plain declaration ~ClassName() override;.
  • All 30 corresponding .cxx files add ClassName::~ClassName() = default; at the top of their namespace block, anchoring the thunk in a single TU with default (exported) visibility.
  • Namespace handling is consistent: namespace itk for most classes, namespace itk::fem for FEMLightObject, and namespace itk::Statistics for the Statistics module classes.
  • Template classes are intentionally left out, as is appropriate — they require explicit instantiation boilerplate to be defined in a .cxx.
  • Three .cxx files (itkExhaustiveOptimizer.cxx, itkSPSAOptimizer.cxx, itkInitializationBiasedParticleSwarmOptimizer.cxx) introduce a cosmetic double blank line after the new destructor definition, because the original code already had one blank line before the first definition and the insertion adds a second.

Confidence Score: 5/5

  • Safe to merge — the fix is mechanically correct and addresses a real ABI defect; only cosmetic style nits remain.
  • All 30 classes are handled identically and correctly. No functional issues were found. The single finding (double blank line in three files) is purely cosmetic and does not affect compilation, runtime behaviour, or ABI correctness.
  • No files require special attention beyond the optional cleanup of the double blank lines in itkExhaustiveOptimizer.cxx, itkSPSAOptimizer.cxx, and itkInitializationBiasedParticleSwarmOptimizer.cxx.

Important Files Changed

Filename Overview
Modules/Core/Common/include/itkEquivalencyTable.h Destructor changed from = default inline to a declaration; definition moved to .cxx to fix ABI visibility.
Modules/Core/Common/src/itkEquivalencyTable.cxx Adds EquivalencyTable::~EquivalencyTable() = default; at the top of the namespace block to anchor destructor thunks with exported visibility.
Modules/Numerics/Optimizers/src/itkExhaustiveOptimizer.cxx Destructor definition added correctly, but leaves a double blank line before the constructor definition (minor style issue).
Modules/Numerics/Optimizers/src/itkSPSAOptimizer.cxx Destructor definition added correctly; introduces a double blank line before the constructor definition (same minor style issue as ExhaustiveOptimizer).
Modules/Numerics/Optimizers/src/itkInitializationBiasedParticleSwarmOptimizer.cxx Destructor definition added correctly; introduces a double blank line before the constructor definition (same minor style issue).
Modules/Numerics/FEM/src/itkFEMLightObject.cxx Correctly adds FEMLightObject::~FEMLightObject() = default; inside namespace itk::fem, consistent with the file's existing namespace style.
Modules/Numerics/Statistics/src/itkChiSquareDistribution.cxx Correctly adds destructor definition inside namespace itk::Statistics; single blank line spacing is consistent.
Modules/Video/Core/src/itkTemporalProcessObject.cxx Correctly adds TemporalProcessObject::~TemporalProcessObject() = default; to anchor destructor thunks.
Modules/Segmentation/Watersheds/src/itkOneWayEquivalencyTable.cxx Correctly moves destructor definition to translation unit; consistent spacing.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Exported class header (.h)\n~ClassName() override = default"] -->|"Before fix\n(-fvisibility-inlines-hidden)"| B["Destructor thunk hidden\n(not exported from DSO)"]
    B --> C["dynamic_cast fails\nacross DSO boundary"]

    D["Exported class header (.h)\n~ClassName() override (declaration only)"] -->|"After fix"| E["Destructor defined in .cxx\nClassName::~ClassName() = default"]
    E --> F["Compiled in one TU\nwith default visibility"]
    F --> G["Destructor thunks exported\nalongside vtable & typeinfo"]
    G --> H["dynamic_cast works correctly\nacross DSO boundary"]

    style C fill:#f88,stroke:#c00
    style H fill:#8f8,stroke:#080
Loading

Reviews (1): Last reviewed commit: "COMP: Move inline = default destructors ..." | Re-trigger Greptile

Comment on lines +22 to 25
ExhaustiveOptimizer::~ExhaustiveOptimizer() = default;


ExhaustiveOptimizer::ExhaustiveOptimizer() = default;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the follow-up commit (STYLE: Fix double blank lines in three Optimizers .cxx files). All three files — itkExhaustiveOptimizer.cxx, itkSPSAOptimizer.cxx, and itkInitializationBiasedParticleSwarmOptimizer.cxx — now have a single blank line between the destructor and constructor definitions.

Non-template exported classes with `~ClassName() override = default;`
inline in the header have hidden D1Ev/D0Ev destructor thunks under
-fvisibility-inlines-hidden, even when the class vtable and typeinfo
remain exported.  This breaks dynamic_cast across DSO boundaries in
shared library builds.

Moving the destructor definition out of line to the .cxx file ensures
it is compiled in exactly one translation unit, giving it default
(exported) visibility and correctly anchoring all destructor thunks.

Affected modules: ITKCommon (3), ITKIOImageBase (3), ITKFEM (1),
ITKOptimizers (11), ITKStatistics (8), ITKWatersheds (2), ITKVideoCore (1).

See: InsightSoftwareConsortium#6000
@hjmjohnson hjmjohnson force-pushed the fix-exported-inline-destructors-abi branch from 9dcf0a1 to d3f17d4 Compare April 1, 2026 19:22
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on a glance.

@dzenanz dzenanz requested a review from N-Dekker April 1, 2026 20:10
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hans!

@hjmjohnson
Copy link
Copy Markdown
Member Author

Thanks Hans!

It was your helpful commit message that made this possible! Thank you for noticing the issue. I used AI to "Create an issue to encapsulate problem that @N-Dekker thinks exists. Investigate the problem with metrics, fix based on findings." ... and then baby-sat and cajoled the solution. :)

@hjmjohnson hjmjohnson merged commit 2fdcea2 into InsightSoftwareConsortium:main Apr 2, 2026
18 checks passed
@hjmjohnson hjmjohnson deleted the fix-exported-inline-destructors-abi branch April 2, 2026 00:56
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 area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Compiler Compiler support or related warnings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants