InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
ENH: Shadow-member sweep — inventory of remaining cases (tracker)
- 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
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