InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

Installed headers leak 44 unprefixed macros into consumer translation units (`COPY`, `RAISE_EXCEPTION`, `NOMINMAX`, ...)

Open
#6,789 0 comments 1 reaction 0 assignees View on GitHub
area:Core area:IO type:Bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.