InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

ENH: Shadow-member sweep — inventory of remaining cases (tracker)

Open
#6,726 0 comments 0 reactions 0 assignees View on GitHub
type:Design type:Enhancement
Dominant language
C++
Stars
1.7k
Forks
748
Avg merge
1d 1h
Merged PRs (30d)
64

Description

Tracker for the tail of the `-Wshadow-field` member-shadow sweep. The mechanical cases landed in #6254, #6255, #6256, #6257 and #6258; **five real cases remain**, each needing per-case design judgment rather than a mechanical edit.

An earlier triage put the remaining count at ~20; re-verification against current `main` (2026-07-29) reduced that to five — the rest were name collisions in nested helper classes, unrelated class hierarchies, or files that no longer exist.

Remaining cases (5) — actionable table

| # | Subclass member | Subclass file:line | Base declaration | Base protection | Class | Why not done |
|---|---|---|---|---|---|---|
| 1 | `m_TempLowerBound`, `m_TempUpperBound`, `m_TempMean` | `Modules/Numerics/Statistics/include/itkWeightedCentroidKdTreeGenerator.h:109-111` | `itkKdTreeGenerator.h:196,199,202` | **private** (`private:` at 179) | HARD | Base member inaccessible. These are algorithm-scratch fields; base and subclass each run their own tree-construction pass, so *sharing* the storage is a design decision, not a cleanup. |
| 2 | `m_Locator`, `m_Marcher` | `Modules/Segmentation/LevelSets/include/itkExtensionVelocitiesImageFilter.h:127,129` | `itkReinitializeLevelSetImageFilter.h:179,181` | **private** (`private:` at 176) | HARD | Base member inaccessible; requires promoting both to `protected:` or routing through protected accessors. |
| 3 | `m_Parameters` | `Modules/IO/TransformMINC/include/itkMINCTransformAdapter.h:317` | `itkTransform.h:576` (`mutable ParametersType`) | **protected** (`protected:` at 561) | MEDIUM | Base is accessible after all — the blocker recorded earlier was wrong. Needs a check that the adapter's use of the parameter array is compatible with the base's `mutable` caching before the shadow is dropped. |
| 4 | `m_DerivativeCalculator` | `Modules/Registration/Common/include/itkMutualInformationImageToImageMetric.h:258` | `itkImageToImageMetric.h:566` | protected | DEFER | Actively read/written on 5 sites in the subclass `.hxx`. Both bind the same `DerivativeFunctionType::Pointer` alias, but the base's algorithm may reset the calculator on a path the subclass also takes — consolidating the two could null a live pointer. Needs an execution-path audit before touching. |
| 5 | `m_ComputeUpdateGPUKernelHandle` | `Modules/Registration/GPUPDEDeformable/include/itkGPUDemonsRegistrationFunction.h:259` | `itkGPUFiniteDifferenceFunction.h:123` | protected | DEFER | Identical `int` type and `{}` default, 11 subclass `.hxx` references. Safe *if* the base's kernel-registration logic uses the same handle slot — unverifiable without a GPU-enabled build, which is why it was parked. |

Tracked separately: `RGBGibbsPriorFilter::m_ClassifierPtr` — base member is private **and** `SetClassifier` is non-virtual, so removing it needs a base-class API change. See the dedicated issue.

Confirmed false positives (14) — do not re-file these

The original scan matched on member-name equality only, ignoring enclosing-class scope, type compatibility, and the actual inheritance edge. That produced 14 false hits:

| Candidate | Why it is not a shadow |
|---|---|
| `BSplineDeformableTransform` | No such declaration in the source — the scan regex matched comment text. |
| `LoadElement::m_Element` | Name reuse with a **different type**: the subclass member is a vector, the base's is a single pointer. Intentional. |
| `ParallelSparseFieldLevelSetImageFilter::m_RMSChange` | Declared inside the nested `ThreadDataUnaligned` struct, not the class. |
| `ImageToListSampleAdaptor` | Member is in a nested `Iterator`/`ConstIterator` class. |
| `PointSetToListSampleAdaptor` | Same nested-iterator pattern. |
| `VectorContainerToListSampleAdaptor` | Same nested-iterator pattern. |
| `ImageToNeighborhoodSampleAdaptor` | Same nested-iterator pattern. |
| `JointDomainImageToListSampleAdaptor` | Same nested-iterator pattern. |
| `TileMergeImageFilter::m_Tiles` | Different `ImagePointer` types — base uses a scalar-pixel `Image`, subclass uses the original `TImageType` (e.g. RGB). Intentional. |
| `WienerDeconvolutionImageFilter::m_KernelZeroMagnitudeThreshold` | Declaration at `itkWienerDeconvolutionImageFilter.h:216` is inside `namespace Functor` (`WienerDeconvolutionFunctor`, line 148), not the filter class (line 80). The functor does not inherit from `InverseDeconvolutionImageFilter`. |
| `TikhonovDeconvolutionImageFilter::m_KernelZeroMagnitudeThreshold` | Identical: `itkTikhonovDeconvolutionImageFilter.h:183` is in `namespace Functor` (line 124); filter class is at line 54. |
| `FastApproximateRankImageFilter::m_Rank` | The filter derives from `MiniPipelineSeparableImageFilter` (`:44-45`), not `RankImageFilter`; `itkMiniPipelineSeparableImageFilter.h` has no `m_Rank`. No inheritance edge. |
| `FEMSpatialObjectReader::m_FileName` | `Modules/Numerics/FEM/include/itkFEMSpatialObjectReader.h` no longer exists in `main`. |
| `FEMSpatialObjectWriter::m_FileName` | Same — file no longer exists. |

**Takeaway for any future scan:** name equality is not sufficient. A scan must verify (a) the declaration's *enclosing* class, (b) that an inheritance edge actually exists between the two enclosing classes, and (c) that the declared types are compatible. Roughly three quarters of the raw hits from a name-only scan were noise.

Per-case safety checklist (apply before deleting any shadow)

1. **Same type?** The subclass often uses a template-parameter alias; confirm it resolves to the base's alias, don't assume.
2. **Same default?** Compare in-class initializers. A subclass-specific default must be preserved by an explicit assignment in the subclass constructor body.
3. **Base member accessible?** Must be `public:` or `protected:`. If `private:`, the fix requires a base-class API change first.
4. **Compatible logical intent?** If the base's algorithm uses the field on a code path that also runs for subclass instances, the separate storage may be deliberate. Read both `.hxx` paths.
5. **`PrintSelf` duplication?** Subclasses frequently re-print fields the base already prints via `Superclass::PrintSelf`. Drop the duplicates along with the shadow.

Methodology used by the merged PRs (repeat it)

1. Branch from `main`; one shadow class per branch.
2. Add a GTest exercising the observable default plus `Set`/`Get` through **both** a subclass pointer and a base pointer.
3. Confirm the test passes against unmodified code — commit 1.
4. Delete the shadow declaration, preserving any subclass-specific default in the constructor body.
5. Confirm the same test plus the module's existing CTests still pass — commit 2.
6. Open one draft PR per shadow class.

Suggested ordering for the remaining five: #3 (`MINCTransformAdapter`, base protected, smallest surface) → #5 (`GPUDemons`, low semantic risk) → #4 (`MutualInformation`, needs execution-path audit) → #1 and #2 (both need a base-class protection bump).

Related work

Merged: #6253 (`itkVirtual`/`itkFinal`/`itkNonVirtual` macros), #6254 (`RGBGibbsPriorFilter` `NumberOfClasses` shadows), #6255 (`MultiGradientOptimizerv4Template`), #6256 (`CurvesLevelSetFunction` orphan shadows), #6257 (`MatchCardinalityImageToImageMetric` `m_Threader`), #6258 (`AntiAliasBinaryImageFilter` `m_InputImage`), #6316 (`-Wshadow-field`).

Contributor guide

Open the contributing guide

Research direction

Start with Modules/IO/TransformMINC/include/itkMINCTransformAdapter.h and itkTransform.h, following the issue’s suggested order from the smallest protected-member case. Use the per-case safety checklist, add a GTest covering defaults and Set/Get through subclass and base pointers, verify the baseline, then run the module’s existing CTests; done means each selected shadow is safely removed with one focused draft PR per class.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.