Skip to content

BUG: Fix two configure probes that never worked and remove obsolete ones - #7123

Merged
hjmjohnson merged 6 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:bug-fenv-and-optimization-probes
Nov 6, 2026
Merged

hjmjohnson merged 6 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:bug-fenv-and-optimization-probes

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

Fix two configure probes that never worked, and remove the probes that C++17 or the minimum compilers already guarantee, from the survey in #7120. Folds in #7124, which touched the same floating-point exception code.

Changes, one commit each
  • BUG: itkFloatingPointExceptions_unix_feenableexcept_using_fegetenv.cxx tested ITK_HAS_STRUCT_FENV_T_CONTROL_CW, but the probe defines ITK_HAS_STRUCT_FENV_T_CW. On a C library that names the fenv_t member __cw, the build reached #error "Unknown name for 'fenv_t' struct control member".
  • COMP: CMake/ITKSetStandardCompilerFlags.cmake passed /Od or -O0 to check_cxx_source_compiles as the program text, so it always failed, and nothing read CXX_HAS_DISABLE_OPTIMIZATION_FLAG. ITK_CXX_DISABLE_OPTIMIZATION_FLAG is unchanged.
  • STYLE: Remove the fenv.h, fegetenv, and fesetenv probes; all three are standard C++11. The fegetenv-based code, reached only on PowerPC and x86 without feenableexcept, is now used there unconditionally. The public ITK_HAVE_FENV_H, always defined and read by nothing in ITK, is removed.
  • COMP: ITK_COMPILED_CXX_STANDARD_VERSION is set to 201703L. The probe it replaces found 201703L everywhere except MSVC, where __cplusplus stays 199711L, so the older-standard warning could never fire there. The warning now reads _MSVC_LANG and uses #pragma message on MSVC.
  • COMP: CMake and supported_compilers.md require AppleClang 12.0.0 (Xcode 12.4), matching the guard in itkMacro.h.
  • DOC: Migration guide note for ITK_HAVE_FENV_H.
Behavior changes to review
  • A project compiled with MSVC at a standard older than C++17 now gets a warning; before, it got nothing.
  • AppleClang 10 and 11 now fail at configure time. They already failed to compile ITK.
  • ITK_HAVE_FENV_H is no longer defined. No use was found in 309 downstream repositories.
Verification
  • Linux x86_64 (glibc, GCC), full build with tests: builds with no warnings; the probe takes ITK_HAS_STRUCT_FENV_T_CONTROL_WORD; 742/742 ITKCommon tests pass, including the floating-point exception, numeric-traits, and math tests.
  • macOS arm64 (earlier, per commit group): ITKCommon and its test drivers build; 91/91 floating-point, numeric-traits, and math tests pass; an x86_64 macOS target compiles the fegetenv path.
  • pre-commit run --all-files passes.
  • Not run: MSVC, or a C library with a __cw member (glibc uses __control_word, macOS __control).
AI assistance

Claude Code (Opus 5.5) surveyed the probes, made the changes, searched the downstream build forest, and ran the local checks. The author reviewed and is responsible for the change.

🤖 Generated with Claude Code

@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:Core Issues affecting the Core module labels Nov 5, 2026
The probe for a fenv_t member named __cw defines
ITK_HAS_STRUCT_FENV_T_CW, but the fegetenv-based floating-point
exception code tested ITK_HAS_STRUCT_FENV_T_CONTROL_CW. On an x86 C
library that names the member __cw, the build stopped at "Unknown name
for 'fenv_t' struct control member".
check_cxx_source_compiles was given "/Od" or "-O0" as the program text,
so it always failed, and nothing read its result. Two test directories
use ITK_CXX_DISABLE_OPTIMIZATION_FLAG itself, which is unchanged.
<cfenv>, fegetenv, and fesetenv are part of C++11, so every toolchain
that can build ITK has them. The fegetenv-based floating-point
exception code is reached only on PowerPC and x86 without
feenableexcept and is now used there unconditionally.

ITK_HAVE_FENV_H was always defined and read by nothing in ITK.
The probe tested only C++11 through C++17 against __cplusplus. On every
supported compiler except MSVC it found 201703L; on MSVC __cplusplus
stays 199711L, so it found nothing and the older-standard warning in
itkConfigure.h could never fire. The value is now ITK's minimum, and
the warning reads _MSVC_LANG and uses #pragma message on MSVC.
itkMacro.h has rejected AppleClang older than Xcode 12.4 since
6771697, while the CMake check and the supported-compilers page
still said 10.0.0. An older AppleClang now fails at configure time
with a clear message instead of at the first ITK header.
The macro was public, so a project that tests it needs to know what to
do instead.
@hjmjohnson
hjmjohnson force-pushed the bug-fenv-and-optimization-probes branch from 20c079b to e82c2a0 Compare November 6, 2026 01:29
@hjmjohnson hjmjohnson changed the title BUG: Fix two configure probes that never worked BUG: Fix two configure probes that never worked and remove obsolete ones Nov 6, 2026
@github-actions github-actions Bot added the area:Documentation Issues affecting the Documentation module label Nov 6, 2026
@hjmjohnson
hjmjohnson marked this pull request as ready for review November 6, 2026 03:21
@greptile-apps

greptile-apps Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5 Tier: plus

[Medium impact] The PR appears safe to merge; no actionable defects were found.

Summary

Fixes the floating-point control-word check and removes obsolete configure probes.

  • Unix floating-point exceptions use the matching fenv_t member probe.
  • MSVC projects below C++17 now get an ITK compatibility warning.
  • ITK requires AppleClang 12.0.0 or newer to configure.
  • ITK no longer publishes the ITK_HAVE_FENV_H macro.
  • CMake no longer runs an unused optimization-flag check.

Review used source inspection; no builds or tests were run.

Reviews (1) · Last reviewed commit: "DOC: Note the removal of ITK_HAVE_FENV_H..." · Reviewed by Greptile

@dzenanz dzenanz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good.

@hjmjohnson
hjmjohnson merged commit ff55a2c into InsightSoftwareConsortium:main Nov 6, 2026
20 checks passed
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request Nov 6, 2026
MSVC keeps __cplusplus at 199711L unless /Zc:__cplusplus is given, so
vendored libraries that test __cplusplus take their C++98 paths under
MSVC. Add the flag to ITK_REQUIRED_CXX_FLAGS beside /bigobj, which applies
it to ITK's build and to UseITK.cmake clients. clang-cl already reports the
right value. Clients built without the flag see the same ITK headers:
itkConfigure.h compares _MSVC_LANG on MSVC, which follows /std: either way.

Suggested by dzenanz in the review of InsightSoftwareConsortium#7123.
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request Nov 8, 2026
MSVC keeps __cplusplus at 199711L unless /Zc:__cplusplus is given, so
vendored libraries that test __cplusplus take their C++98 paths under
MSVC. Add the flag to ITK_REQUIRED_CXX_FLAGS beside /bigobj, which applies
it to ITK's build and to UseITK.cmake clients. clang-cl already reports the
right value. Clients built without the flag see the same ITK headers:
itkConfigure.h compares _MSVC_LANG on MSVC, which follows /std: either way.

Suggested by dzenanz in the review of InsightSoftwareConsortium#7123.
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request Nov 8, 2026
MSVC keeps __cplusplus at 199711L unless /Zc:__cplusplus is given, so
vendored libraries that test __cplusplus take their C++98 paths under
MSVC. Append the flag to CMAKE_CXX_FLAGS with ITK's warning and
optimization flags, so it applies to ITK's own build and is not exported
through ITK_REQUIRED_CXX_FLAGS. Clients do not need it: itkConfigure.h
compares _MSVC_LANG on MSVC, which follows /std: either way. clang-cl
already reports the right value.

Suggested by dzenanz in the review of InsightSoftwareConsortium#7123.

Co-Authored-By: Bradley Lowekamp <blowekamp@mail.nih.gov>
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 type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants