Remove old CMake code about gcc version check for -frounding-math (#6923 )#9356
Remove old CMake code about gcc version check for -frounding-math (#6923 )#9356RajdeepKushwaha5 wants to merge 2 commits intoCGAL:mainfrom
Conversation
…6923) The regex "${GCC_VERSION}" MATCHES "^[4-9]." only matches single-digit GCC major versions (4 through 9), silently failing for GCC 10 and later. The flag was therefore never added for any modern GCC version via this code path. This code is also redundant: CGAL_SetupCGALDependencies.cmake already adds -frounding-math correctly using CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 3 via target_compile_options() on the CGAL interface target, which is the authoritative mechanism for this flag.
There was a problem hiding this comment.
Pull request overview
This PR removes a redundant/incorrect GCC-version regex block from Installation/CMakeLists.txt that attempted to add -frounding-math, relying instead on the existing target-based mechanism that handles compiler versions correctly.
Changes:
- Removed the legacy GCC version regex check that added
-frounding-mathviaCGAL_CXX_FLAGS. - Eliminated an outdated status message and the associated dead code path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
A lot of code around those lines should be removed: obsolete and unused. I will try to review that. |
lrineau
added
the
Not yet approved
Thanks for the note! I'd be happy to expand this PR to also remove the SunPro, Intel Classic (icl/icpc), and Alpha architecture dead code blocks if that would help. Alternatively, I can keep this PR focused on the -frounding-math dead code and you can handle the broader cleanup separately — whatever you prefer. |
|
If I remember well, all that code was indeed moved to |
All compiler flag logic for MSVC, SunPro, Intel Classic (icl/icpc), and GCC was already migrated to the modern target-based mechanism in Installation/cmake/modules/CGAL_SetupCGALDependencies.cmake, which uses target_compile_options()/target_link_libraries() INTERFACE. The old blocks used uniquely_add_flags(CGAL_CXX_FLAGS ...) which is the obsolete global-variable approach. Removed dead blocks: - MSVC: /fp:strict, /fp:except-, /bigobj, _SCL_SECURE_NO_DEPRECATE, CMAKE_CXX_WARNING_LEVEL (all superseded by target_compile_options in CGAL_setup_CGAL_flags()) - SunPro: -features=extensions, -library=stlport4, -D_GNU_SOURCE, linker flags (superseded by target_compile_options + target_link_libraries in CGAL_setup_CGAL_flags()) - Intel Classic (icl/icpc): /fp:strict / -fp-model=strict, version check for < 11.0 (superseded; Intel < 11.0 from 2008 cannot support the C++17 that CGAL now requires) - GCC: -Wall for test suite, -frounding-math regex check, -g stripping from CMAKE_CXX_FLAGS, -mieee -mfp-rounding-mode=d for alpha (all superseded by target_compile_options in CGAL_setup_CGAL_flags()) The entire empty FLAGS section header is also removed. Closes CGAL#6923
afabri
added
Ready to be tested
and removed
Not yet approved
Root Cause:
Installation/CMakeLists.txtcontained a dead code block that attempted to add-frounding-mathfor GCC using:The regex
^[4-9].only matches single-digit GCC major versions (4-9). GCC 10, 11, 12, 13, 14, etc. do NOT match, so the flag was silently never added via this path for any modern GCC.Furthermore, this code is entirely redundant: the correct mechanism is in
Installation/cmake/modules/CGAL_SetupCGALDependencies.cmake, which uses:This correctly handles all GCC 4+ via CMake's modern target_compile_options interface mechanism.
Fix:
Removed the 4-line dead block from
Installation/CMakeLists.txt.Files changed:
Installation/CMakeLists.txt- removed 5 lines (4 code + 1 blank)