Skip to content

ENH: Determine the byte order in ByteSwapper and retire the config macros - #7112

Open
hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-retire-cmake-words-bigendian
Open

hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-retire-cmake-words-bigendian

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Remove the byte order from the public configuration header: itk::ByteSwapper<T>::SystemIsBigEndian() now determines it from the compiler (std::endian in C++20, otherwise the compiler's own macros), the three in-tree users call it, the CMake detection is removed, and CMAKE_WORDS_BIGENDIAN / ITK_WORDS_BIGENDIAN are defined only without ITK_LEGACY_REMOVE. Follows #7101.

Why

itkConfigure.h published two macros that no project outside ITK uses (0 of 309 repositories in the downstream forest testbed, and only vendored ITK copies on GitHub), one of them under a CMake-prefixed name. A byte order detected at configure time also describes the building machine, which is wrong for one architecture of a universal binary or for a cross-compile; the compiler always knows the byte order of the architecture it is compiling for. The two names stay as a deprecation path under !ITK_LEGACY_REMOVE, defined from the compiler macros, and the migration guide names the replacement.

Local verification (macOS arm64, Homebrew LLVM 23)
  • Legacy kept: byte-swap and PNG I/O tests 40/40, ImageFusion 24/24 (including ScalarToRGBPixelFunctor.ConvertedLegacyTest).
  • ITK_LEGACY_REMOVE=ON and ITK_FUTURE_LEGACY_REMOVE=ON: Common, IOPNG and ImageFusion build; 62/62 of the same tests pass; the generated itkConfigure.h mentions the two names only inside the legacy block.
  • A new ByteSwap GoogleTest checks SystemIsBigEndian() against the in-memory byte layout of a 16-bit value.
  • PNG I/O fails at compile time, instead of skipping the swap, when libpng lacks 16-bit swap support on a little-endian target.
  • pre-commit run --all-files passes.
  • Not run here: a Windows build (MSVC has no __BYTE_ORDER__ and takes the _WIN32 branch) and a full forest build; CI covers the first.
AI assistance

Claude Code (Fable 5.1) drafted the change and the guide entry, ran the local builds and tests, and searched the forest testbed for users of the macros. The author reviewed and is responsible for the change.

@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Documentation Issues affecting the Documentation module labels Nov 4, 2026
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push bbb2e18 -> fe1a4c5: migration-guide wording only (explicit before/after replacement patterns); no code change.

@hjmjohnson
hjmjohnson force-pushed the enh-retire-cmake-words-bigendian branch from bbb2e18 to fe1a4c5 Compare November 4, 2026 21:34
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push fe1a4c5 -> a8c9864, one real fix: the #if !defined(ITK_LEGACY_REMOVE) block sat above #cmakedefine ITK_LEGACY_REMOVE in itkConfigure.h.in, so it was always true and a big-endian ITK_LEGACY_REMOVE=ON build would still have defined both macros. The block now follows the existing ITK_HAS_CPP11_TYPETRAITS legacy block. Verified by preprocessing the generated header with -D__BIG_ENDIAN__: both macros with ITK_LEGACY_REMOVE=OFF, neither with ON. Also: guide wording and #include lines in the examples, and a redundant comment removed in itkByteSwapper.h.

@hjmjohnson
hjmjohnson force-pushed the enh-retire-cmake-words-bigendian branch from fe1a4c5 to a8c9864 Compare November 4, 2026 21:48
@hjmjohnson
hjmjohnson marked this pull request as ready for review November 5, 2026 01:13
@greptile-apps

greptile-apps Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 4/5

[High impact] The PR changes shared byte-order logic used across ITK's binary I/O workflows.

Fix the system-libpng build regression before merging.

Findings

  1. P1 Some system PNG builds fail ▶
  2. P2 Wrong byte order escapes tests ▶

Summary

Moves byte-order detection into ByteSwapper and retires the public configuration macros.

  • ByteSwapper reads byte order from the target compiler.
  • Scalar-to-RGB colors follow the target's byte order.
  • PNG reads and writes swap 16-bit samples for little-endian targets.

The removal of the macros under ITK_LEGACY_REMOVE is explicitly intentional and documented by hjmjohnson.

Reviews (1) · Last reviewed commit: "ENH: Determine the byte order in ByteSwa..." · Reviewed by Greptile

Comment thread Modules/IO/PNG/src/itkPNGImageIO.cxx Resolved
…cros

itkConfigure.h exposed the byte order detected at configure time as
CMAKE_WORDS_BIGENDIAN and ITK_WORDS_BIGENDIAN, which no project in the
downstream forest testbed uses, and a configure-time value is wrong for
one architecture of a universal binary. ByteSwapper<T>::SystemIsBigEndian()
now takes the byte order from the compiler (std::endian in C++20,
otherwise its own macros), the three in-tree users call it, the CMake
detection is removed, and both macros remain only without
ITK_LEGACY_REMOVE.
@hjmjohnson
hjmjohnson force-pushed the enh-retire-cmake-words-bigendian branch from a8c9864 to 174c206 Compare November 5, 2026 04:15
@github-actions github-actions Bot added the type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct label Nov 5, 2026
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push a8c9864 -> 174c206, content only on the same base: addresses the two Greptile findings (libpng swap-feature guard in itkPNGImageIO.cxx, and a byte-layout test for SystemIsBigEndian()). No other change.

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 area:Documentation Issues affecting the Documentation module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots 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.

2 participants