InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
Installed headers leak 44 unprefixed macros into consumer translation units (`COPY`, `RAISE_EXCEPTION`, `NOMINMAX`, ...)
- Dominant language
- C++
- Stars
- 1.7k
- Forks
- 748
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 64
Description
ITK's installed headers define **44 unprefixed macros that are never `#undef`'d**, so they are injected into every consumer translation unit that includes them. Several have names a downstream project could plausibly use itself, and one — `COPY` — expands to nothing.
The immediate trigger was `RAISE_EXCEPTION`, defined in two installed IO headers with no cleanup. A tree-wide audit found it is not an isolated case.
**The pattern to follow already exists in ITK:** `ITK_CONVERT_BUFFER_IF_BLOCK` is defined and then `#undef`'d in both `itkImageFileReader.hxx` and `itkVideoFileReader.hxx`. Every macro below either needs that treatment or an `ITK`/`itk` prefix.
Highest risk — generic names, no prefix, no #undef
| Macro | Header | Why it matters |
|---|---|---|
| `COPY` | `Filtering/MathematicalMorphology/include/itkReconstructionImageFilter.h:28` | **Empty object-like macro** (`#define COPY` with no replacement). Any downstream identifier named `COPY` is silently deleted. |
| `TYPENAME(x)` | `IO/ImageBase/include/itkImageFileReader.hxx:498` | Function-like, extremely generic name |
| `RAISE_EXCEPTION()` | `IO/IOFDF/include/itkFDFImageIO.h:139` | Generic; also duplicated |
| `RAISE_EXCEPTION()` | `IO/IPL/include/itkIPLCommonImageIO.h:200` | Same macro, second definition |
| `NOMINMAX` | `Registration/RANSAC/include/nanoflann.hpp:66` | Changes `windows.h` behavior for the whole consumer TU |
| `UNDEFINED_REGION` | `Segmentation/LevelSetsv4/include/itkLevelSetBase.hxx` | |
| `DEBUG_EXECUTE` | `Filtering/ImageGrid/include/itkOrientImageFilter.hxx` | |
| `IOCHECK` | `IO/IPL/include/itkIPLCommonImageIO.h` | |
| `TOP_`, `BOTTOM_`, `LEFT_`, `RIGHT_` | `Filtering/Path/include/itkContourExtractor2DImageFilter.hxx` | Four single-word macros |
| `SUPPORT_TOOLHELP32` | `Core/Common/include/itkMemoryUsageObserver.h` | |
| `FFTWPathSep` | `Filtering/FFT/include/itkFFTWGlobalConfiguration.h` | |
| `TYPENAME_VideoFileReader` | `Video/IO/include/itkVideoFileReader.hxx` | |
`COPY` and `NOMINMAX` are the two that can change the meaning of unrelated downstream code rather than merely collide.
Deliberate public test API — probably intentional, worth a decision
`Core/TestKernel/include/itkTestingMacros.h` exports 11 unprefixed macros:
```
EXERCISE_BASIC_OBJECT_METHODS TEST_EXPECT_EQUAL TEST_EXPECT_EQUAL_STATUS_VALUE
TEST_EXPECT_TRUE TEST_EXPECT_TRUE_STATUS_VALUE
TEST_SET_GET TEST_SET_GET_BOOLEAN TEST_SET_GET_NULL_VALUE
TEST_SET_GET_VALUE TRY_EXPECT_EXCEPTION TRY_EXPECT_NO_EXCEPTION
```
These are meant to be used by test code, including downstream test code, so the lack of a prefix may be deliberate. Also `LOCAL_ITK_TEST_SET_GET_VALUE` in `IO/MeshBase/include/itkMeshIOTestHelper.h`.
If they are intentional public API they should be documented as such and excluded from any future check; if not, they want `ITK_` prefixes with deprecated aliases.
Module-specific helpers — distinctive names, lower risk
```
GetSetFunctorMacro Filtering/AnisotropicDiffusionLBR/…/itkAnisotropicDiffusionLBRMacro.h
GradientAnisotropicDiffusionImageFilterTypeMacro Filtering/GPUAnisotropicSmoothing/…
IPLGetMacroDeclaration / IPLGetMacroDefinition IO/IPL/include/itkIPLFileNameList.h
IPLSetMacroDeclaration / IPLSetMacroDefinition IO/IPL/include/itkIPLFileNameList.h
NANOFLANN_VERSION Registration/RANSAC/include/nanoflann.hpp
OverrideDemonsRegistrationFilterTypeMacro Registration/GPUPDEDeformable/…
OverrideImageTypeMacro Core/GPUCommon/include/itkGPUImage.h
OverrideMeanFilterTypeMacro Filtering/GPUSmoothing/…
OverrideThresholdFilterTypeMacro Filtering/GPUThresholding/…
ScancoGetConstMacro / ScancoSetMacro IO/IOScanco/include/itkScancoDataManipulation.h
CLANG_PRAGMA_PUSH / CLANG_PRAGMA_POP Core/Common/include/itkMacro.h
CLANG_SUPPRESS_Wcpp14_extensions Core/Common/include/itkMacro.h
CLANG_SUPPRESS_Wfloat_equal Core/Common/include/itkMacro.h
```
Collision risk is low, but they are still unconditional additions to every consumer's macro namespace. The `CLANG_*` ones in `itkMacro.h` reach essentially every ITK consumer.
How the audit was run
```python
# every #define in an installed header (Modules/**/include/**, ThirdParty and test excluded),
# minus include guards, minus anything ITK/itk-prefixed, minus anything #undef'd in the same file
git grep -n -E '^[[:space:]]*#[[:space:]]*define[[:space:]]+[A-Za-z_][A-Za-z0-9_]*' \
-- Modules ':!Modules/ThirdParty/*'
```
3,284 installed headers define at least one macro; 44 survive the filter.
Note for anyone reproducing: the git pathspec `Modules/*/*/include/` matches **nothing** — `*` does not cross `/` in git's matcher — so filter paths after the grep rather than in the pathspec. A pathspec that silently matches nothing returns a clean, entirely false, zero.
Suggested approach
1. Start with `COPY` and `NOMINMAX` — these change the meaning of downstream code rather than merely risking a collision.
2. For each macro used only inside its defining header, add `#undef` at the end of the header, matching `ITK_CONVERT_BUFFER_IF_BLOCK`.
3. For macros that must remain visible, rename with an `ITK_`/`itk` prefix.
4. Decide explicitly whether `itkTestingMacros.h` exports are intentional public API. If yes, document and exempt them.
5. Consider a CI check: any `#define` in an installed header must be prefixed, be an include guard, or be `#undef`'d in the same file. This is mechanically checkable and would keep the count at zero.
Step 5 is the durable fix — the other four are one-time cleanups that will otherwise regress.
Contributor guide
Research direction
Start with the cited COPY and NOMINMAX definitions, then compare their headers with itkImageFileReader.hxx and itkVideoFileReader.hxx, where ITK_CONVERT_BUFFER_IF_BLOCK is undefined. Reproduce the installed-header grep audit and inspect the remaining listed macros. Done means unprefixed macros are cleaned up or explicitly justified, with the test macros' public-API status decided and a regression check considered.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend-api-design, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100