Skip to content

COMP: Fix PrintNumericTrait build error in debug mode#6280

Merged
blowekamp merged 2 commits into
InsightSoftwareConsortium:mainfrom
blowekamp:fix-PrintHelper-remove-from-macro-chain
May 15, 2026
Merged

COMP: Fix PrintNumericTrait build error in debug mode#6280
blowekamp merged 2 commits into
InsightSoftwareConsortium:mainfrom
blowekamp:fix-PrintHelper-remove-from-macro-chain

Conversation

@blowekamp
Copy link
Copy Markdown
Member

Fix a build error in debug mode where PrintNumericTrait's os << indent could not resolve operator<<(ostream, const Indent&).

Root cause and approach

itkIndent.h previously pulled in itkMacro.h, which inside #ifndef NDEBUG included itkPrintHelper.h before class Indent was complete. Moving the include to the unconditional end of itkMacro.h breaks the circular chain cleanly.

To enforce the required include order, the #include "itkMacro.h" in itkIndent.h is replaced with a #error guard — this surfaces any future violation at compile time rather than silently producing a bad include order.

A handful of headers that directly included itkIndent.h before itkMacro.h were fixed to include itkMacro.h first. The now-redundant class Indent; forward declaration is removed from itkPrintHelper.h.

Verified by building the full ITK debug tree (cmake --preset debug) with no errors.

AI assistance

GitHub Copilot (Claude Sonnet 4.6) identified the circular include chain, proposed the fix, and iterated until the full debug build was clean.

@github-actions github-actions Bot added type:Compiler Compiler support or related warnings type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels May 14, 2026
@blowekamp blowekamp force-pushed the fix-PrintHelper-remove-from-macro-chain branch from c4e7f7f to 8a1abb8 Compare May 14, 2026 15:41
@blowekamp blowekamp requested a review from seanm May 14, 2026 15:42
@hjmjohnson
Copy link
Copy Markdown
Member

@greptile review

@greptile-apps

This comment was marked as resolved.

@blowekamp
Copy link
Copy Markdown
Member Author

The PR description's AI assistance block names the AI model used, which prose-budget.md explicitly forbids.

git-commits.md explicit requests inclusion of this in the PR description.

Remove itkPrintHelper.h from the #ifndef NDEBUG block in itkMacro.h
and include it unconditionally at the end of itkMacro.h instead.
Replace the itkMacro.h #include in itkIndent.h with a #error guard
that enforces the required include order. Fix the handful of headers
that included itkIndent.h before itkMacro.h. Remove the now-redundant
class Indent forward declaration from itkPrintHelper.h.
@blowekamp blowekamp force-pushed the fix-PrintHelper-remove-from-macro-chain branch from 8a1abb8 to 79f01ca Compare May 14, 2026 18:18
The #error guard broke standalone header inclusion tested by
ITKCommonHeaderTest1. itkIndent.h only needs ITKCommon_EXPORT,
so include ITKCommonExport.h directly to avoid the circular
dependency through itkMacro.h -> itkPrintHelper.h -> itkIndent.h.
@hjmjohnson hjmjohnson marked this pull request as ready for review May 14, 2026 22:58
@hjmjohnson hjmjohnson marked this pull request as draft May 14, 2026 22:59
@blowekamp blowekamp marked this pull request as ready for review May 15, 2026 10:07
@blowekamp blowekamp merged commit 63f6ddc into InsightSoftwareConsortium:main May 15, 2026
23 of 28 checks passed
@hjmjohnson
Copy link
Copy Markdown
Member

@blowekamp Thank you. I'll now layer in the CI that adds a debug build.

hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 15, 2026
Repurpose the ITK.Linux pipeline (the only non-Python per-PR Azure
pipeline with wall-time headroom) from MinSizeRel to Debug while
keeping BUILD_EXAMPLES=ON, adding per-PR Debug-mode coverage without
a new pipeline.  Companion to InsightSoftwareConsortium#6279 (missing-examples fixes) and
relies on InsightSoftwareConsortium#6280 (Debug itkPrintHelper include-order fix, merged).

Changes:
  * CTEST_BUILD_CONFIGURATION MinSizeRel -> Debug; job/suffix/test-run
    renamed *DebugCxx20 so CDash and ccache keys stay partitioned.
  * EXCLUDE_LABEL extended to "BigIO|RUNS_LONG" to bound Debug test
    wall time.
  * CCACHE_MAXSIZE 2.4G -> 6.25G.  Empirical: non-Debug ITK.Linux warm
    cache is ~1.6 GiB; Debug objects are several times larger, and the
    sibling ITK.Linux.Python pipeline already sustains a ~4.7-6.2 GB
    cache on the same agent class, so 6.25G is the validated ceiling
    with disk headroom.
  * Post-build cache trim: record build start, run build+test, then
    'ccache --evict-older-than ${BUILD_DURATION}s'.  ccache refreshes
    an entry timestamp on every hit, so last-use eviction keeps only
    this build's working set in the saved cache, capping the artifact
    near a single Debug build instead of the cross-PR high-water mark.

Trade: MinSizeRel binary-size monitoring is dropped (rarely surfaces
issues Release does not); per-PR Debug semantics (assert(), debug
iterators, debug-mode template instantiations) is higher value.
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 15, 2026
Repurpose the ITK.Linux pipeline (the only non-Python per-PR Azure
pipeline with wall-time headroom) from MinSizeRel to Debug while
keeping BUILD_EXAMPLES=ON, adding per-PR Debug-mode coverage without
a new pipeline.  Companion to InsightSoftwareConsortium#6279 (missing-examples fixes) and
relies on InsightSoftwareConsortium#6280 (Debug itkPrintHelper include-order fix, merged).

Changes:
  * CTEST_BUILD_CONFIGURATION MinSizeRel -> Debug; job/suffix/test-run
    renamed *DebugCxx20 so CDash and ccache keys stay partitioned.
  * EXCLUDE_LABEL extended to "BigIO|RUNS_LONG" to bound Debug test
    wall time.
  * CCACHE_MAXSIZE 2.4G -> 6.25G.  Empirical: non-Debug ITK.Linux warm
    cache is ~1.6 GiB; Debug objects are several times larger, and the
    sibling ITK.Linux.Python pipeline already sustains a ~4.7-6.2 GB
    cache on the same agent class, so 6.25G is the validated ceiling
    with disk headroom.
  * Post-build cache trim: record build start, run build+test, then
    'ccache --evict-older-than ${BUILD_DURATION}s'.  ccache refreshes
    an entry timestamp on every hit, so last-use eviction keeps only
    this build's working set in the saved cache, capping the artifact
    near a single Debug build instead of the cross-PR high-water mark.

Trade: MinSizeRel binary-size monitoring is dropped (rarely surfaces
issues Release does not); per-PR Debug semantics (assert(), debug
iterators, debug-mode template instantiations) is higher value.
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 15, 2026
Repurpose the ITK.Linux pipeline (the only non-Python per-PR Azure
pipeline with wall-time headroom) from MinSizeRel to Debug while
keeping BUILD_EXAMPLES=ON, adding per-PR Debug-mode coverage without
a new pipeline.  Companion to InsightSoftwareConsortium#6279 (missing-examples fixes) and
relies on InsightSoftwareConsortium#6280 (Debug itkPrintHelper include-order fix, merged).

Changes:
  * CTEST_BUILD_CONFIGURATION MinSizeRel -> Debug; job/suffix/test-run
    renamed *DebugCxx20 so CDash and ccache keys stay partitioned.
  * EXCLUDE_LABEL extended to "BigIO|RUNS_LONG" to bound Debug test
    wall time.
  * CCACHE_MAXSIZE 2.4G -> 6.25G.  Empirical: non-Debug ITK.Linux warm
    cache is ~1.6 GiB; Debug objects are several times larger, and the
    sibling ITK.Linux.Python pipeline already sustains a ~4.7-6.2 GB
    cache on the same agent class, so 6.25G is the validated ceiling
    with disk headroom.
  * Post-build cache trim: record build start, run build+test, then
    'ccache --evict-older-than ${BUILD_DURATION}s'.  ccache refreshes
    an entry timestamp on every hit, so last-use eviction keeps only
    this build's working set in the saved cache, capping the artifact
    near a single Debug build instead of the cross-PR high-water mark.

Trade: MinSizeRel binary-size monitoring is dropped (rarely surfaces
issues Release does not); per-PR Debug semantics (assert(), debug
iterators, debug-mode template instantiations) is higher value.
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 16, 2026
Repurpose the ITK.Linux pipeline (the only non-Python per-PR Azure
pipeline with wall-time headroom) from MinSizeRel to Debug while
keeping BUILD_EXAMPLES=ON, adding per-PR Debug-mode coverage without
a new pipeline.  Companion to InsightSoftwareConsortium#6279 (missing-examples fixes) and
relies on InsightSoftwareConsortium#6280 (Debug itkPrintHelper include-order fix, merged).

Changes:
  * CTEST_BUILD_CONFIGURATION MinSizeRel -> Debug; job/suffix/test-run
    renamed *DebugCxx20 so CDash and ccache keys stay partitioned.
  * EXCLUDE_LABEL extended to "BigIO|RUNS_LONG" to bound Debug test
    wall time.
  * CCACHE_MAXSIZE 2.4G -> 6.25G.  Empirical: non-Debug ITK.Linux warm
    cache is ~1.6 GiB; Debug objects are several times larger, and the
    sibling ITK.Linux.Python pipeline already sustains a ~4.7-6.2 GB
    cache on the same agent class, so 6.25G is the validated ceiling
    with disk headroom.
  * Post-build cache trim: record build start, run build+test, then
    'ccache --evict-older-than ${BUILD_DURATION}s'.  ccache refreshes
    an entry timestamp on every hit, so last-use eviction keeps only
    this build's working set in the saved cache, capping the artifact
    near a single Debug build instead of the cross-PR high-water mark.

Trade: MinSizeRel binary-size monitoring is dropped (rarely surfaces
issues Release does not); per-PR Debug semantics (assert(), debug
iterators, debug-mode template instantiations) is higher value.
@issakomi
Copy link
Copy Markdown
Member

issakomi commented May 16, 2026

May be unrelated. It is somewhat unusual that itkIndent.cxx doesn't have #include "itkIndent.h", it was removed 15 years ago together with many "unnecessary headers" in 573920f. itkIndent.cxx only has one header #include "itkObjectFactory.h". Just FYI.

hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 16, 2026
Repurpose the ITK.Linux pipeline (the only non-Python per-PR Azure
pipeline with wall-time headroom) from MinSizeRel to Debug while
keeping BUILD_EXAMPLES=ON, adding per-PR Debug-mode coverage without
a new pipeline.  Companion to InsightSoftwareConsortium#6279 (missing-examples fixes) and
relies on InsightSoftwareConsortium#6280 (Debug itkPrintHelper include-order fix, merged).

Changes:
  * CTEST_BUILD_CONFIGURATION MinSizeRel -> Debug; job/suffix/test-run
    renamed *DebugCxx20 so CDash and ccache keys stay partitioned.
  * EXCLUDE_LABEL extended to "BigIO|RUNS_LONG" to bound Debug test
    wall time.
  * CCACHE_MAXSIZE 2.4G -> 6.25G.  Empirical: non-Debug ITK.Linux warm
    cache is ~1.6 GiB; Debug objects are several times larger, and the
    sibling ITK.Linux.Python pipeline already sustains a ~4.7-6.2 GB
    cache on the same agent class, so 6.25G is the validated ceiling
    with disk headroom.
  * Post-build cache trim: record build start, run build+test, then
    'ccache --evict-older-than ${BUILD_DURATION}s'.  ccache refreshes
    an entry timestamp on every hit, so last-use eviction keeps only
    this build's working set in the saved cache, capping the artifact
    near a single Debug build instead of the cross-PR high-water mark.

Trade: MinSizeRel binary-size monitoring is dropped (rarely surfaces
issues Release does not); per-PR Debug semantics (assert(), debug
iterators, debug-mode template instantiations) is higher value.
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 type:Compiler Compiler support or related warnings type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants