From 2538fd38e1241e670499dd79dcaee985011ffe23 Mon Sep 17 00:00:00 2001 From: Joe Rivera Date: Fri, 9 Oct 2026 16:41:33 +0000 Subject: [PATCH] fix(headers): compile after / -- rename world_units' si_value, macro-proof MIN/MAX glibc's defines `#define si_value _sifields._rt.si_sigval`, so any TU that includes it before fails: world_units.h:147:28: error: expected ',' or '...' before '.' token Seen building volrover3's GeometryDialog/IsosurfaceDialog/ ProceduralGeometryDialog/SDFDialog against the installed 3.5.0 prefix (transfix/volrover#48). - world_units::to_display/format: rename the `si_value` parameter to `canonical_value` (pairs with from_display's `display_value`; no si_* prefix, which glibc/musl reserve for siginfo_t accessor macros). Parameter names only, so no ABI/API change; docs updated to match. - utility.h: an audit of every identifier in inc/ against the macros defined by ~50 glibc/POSIX headers found one more clash: the cvc::MIN / cvc::MAX templates break after (also macOS, GLib), whose function-like MIN(a,b)/MAX(a,b) expand the declarations. Parenthesize the declarator names; existing callers are unchanged. - New system_macro_headers_test: includes , , , (POSIX) or (the repo's WIN32_LEAN_AND_MEAN + NOMINMAX pattern) before a representative public header from every cvc module, so a regression fails the build. --- docs/WORLD_UNITS_API.md | 8 +- inc/cvc/core/world_units.h | 4 +- inc/cvc/utility/utility.h | 7 +- src/cvc/core/world_units.cpp | 12 +-- src/cvc/tests/CMakeLists.txt | 11 +++ src/cvc/tests/system_macro_headers_test.cpp | 87 +++++++++++++++++++++ 6 files changed, 115 insertions(+), 14 deletions(-) create mode 100644 src/cvc/tests/system_macro_headers_test.cpp diff --git a/docs/WORLD_UNITS_API.md b/docs/WORLD_UNITS_API.md index 30410b76b..a8e665bae 100644 --- a/docs/WORLD_UNITS_API.md +++ b/docs/WORLD_UNITS_API.md @@ -147,9 +147,9 @@ double metres_to_world(double metres) const; // metres / scale (scale > 0 ### Canonical SI ↔ the display regime ```cpp -double to_display(double si_value, dimension d) const; // SI -> regime base unit +double to_display(double canonical_value, dimension d) const; // SI -> regime base unit double from_display(double display_value, dimension d) const; // regime base unit -> SI -std::string unit_symbol(dimension d) const; // "m"/"ft", "kg"/"lb", ... +std::string unit_symbol(dimension d) const; // "m"/"ft", "kg"/"lb", ... ``` `to_display`/`from_display` use the regime's **base** unit (metre/foot, @@ -161,7 +161,7 @@ kilogram/pound, m/s / mph, …). They do **not** promote by magnitude — use ```cpp struct measurement { double value; std::string unit; }; -measurement format(double si_value, dimension d) const; +measurement format(double canonical_value, dimension d) const; struct coordinate { double x, y, z; std::string unit; }; coordinate world_point_to_real(double wx, double wy, double wz) const; @@ -245,7 +245,7 @@ package). `world_units` is what a future Jolt bridge reads: velocities and forces cross the engine boundary in canonical SI with **no scaling**. Only `metres_per_world_unit` is applied when importing world-space geometry into the physics world. -- Report engine outputs to the user with `format(si_value, dimension::force)` +- Report engine outputs to the user with `format(canonical_value, dimension::force)` etc., so a change of regime converts every reported quantity through the same table. - Keep conversions **out** of the bit-identical nav kernels (they are compiled diff --git a/inc/cvc/core/world_units.h b/inc/cvc/core/world_units.h index 9ac0fd223..a5e6cc558 100644 --- a/inc/cvc/core/world_units.h +++ b/inc/cvc/core/world_units.h @@ -144,7 +144,7 @@ class world_units { // of the active regime expects, in that regime's BASE unit (metre/foot, // kilogram/pound, second, m/s / mph, ...), and back. No magnitude-based // promotion happens here; use format() for that. - double to_display(double si_value, dimension d) const; + double to_display(double canonical_value, dimension d) const; double from_display(double display_value, dimension d) const; // The base unit label for a dimension in the active regime, e.g. "m"/"ft", @@ -160,7 +160,7 @@ class world_units { double value = 0.0; std::string unit; }; - measurement format(double si_value, dimension d) const; + measurement format(double canonical_value, dimension d) const; // --- coordinate mapping ------------------------------------------------ diff --git a/inc/cvc/utility/utility.h b/inc/cvc/utility/utility.h index c907f6f28..86e1d78e7 100644 --- a/inc/cvc/utility/utility.h +++ b/inc/cvc/utility/utility.h @@ -74,8 +74,11 @@ static inline bool ends_with(const std::string &haystack, const std::string &nee // in the .cpp file but the mac was complaining const uint64 VM_VERSION = 0x00010206; -template const T &MIN(const T &a, const T &b) { return std::min(a, b); } -template const T &MAX(const T &a, const T &b) { return std::max(a, b); } +// The names are parenthesized so a function-like MIN(a,b)/MAX(a,b) macro (glibc and +// macOS , GLib) cannot expand the declarations when such a header is +// included first. +template const T &(MIN)(const T &a, const T &b) { return std::min(a, b); } +template const T &(MAX)(const T &a, const T &b) { return std::max(a, b); } // trilinear interpolation function static inline double getTriVal(double val[8], double x, double y, double z, double resX, diff --git a/src/cvc/core/world_units.cpp b/src/cvc/core/world_units.cpp index e7b7b4b33..41f851d1d 100644 --- a/src/cvc/core/world_units.cpp +++ b/src/cvc/core/world_units.cpp @@ -163,13 +163,13 @@ double world_units::metres_to_world(double metres) const { return metres / scale; } -double world_units::to_display(double si_value, dimension d) const { +double world_units::to_display(double canonical_value, dimension d) const { system regime; { std::lock_guard lk(_mutex); regime = _cfg.regime; } - return si_value / si_per_display_unit(d, regime); + return canonical_value / si_per_display_unit(d, regime); } double world_units::from_display(double display_value, dimension d) const { @@ -190,7 +190,7 @@ std::string world_units::unit_symbol(dimension d) const { return base_unit_symbol(d, regime); } -world_units::measurement world_units::format(double si_value, dimension d) const { +world_units::measurement world_units::format(double canonical_value, dimension d) const { system regime; { std::lock_guard lk(_mutex); @@ -198,11 +198,11 @@ world_units::measurement world_units::format(double si_value, dimension d) const } measurement m; if (d == dimension::length) { - const length_tier tier = pick_length_tier(si_value, regime); - m.value = si_value / tier.metres_per_unit; + const length_tier tier = pick_length_tier(canonical_value, regime); + m.value = canonical_value / tier.metres_per_unit; m.unit = tier.unit; } else { - m.value = si_value / si_per_display_unit(d, regime); + m.value = canonical_value / si_per_display_unit(d, regime); m.unit = base_unit_symbol(d, regime); } return m; diff --git a/src/cvc/tests/CMakeLists.txt b/src/cvc/tests/CMakeLists.txt index ac169fefe..28b9801c1 100644 --- a/src/cvc/tests/CMakeLists.txt +++ b/src/cvc/tests/CMakeLists.txt @@ -89,6 +89,7 @@ add_executable(state_hash_partition_test state_hash_partition_test.cpp) add_executable(state_hybrid_time_test state_hybrid_time_test.cpp) add_executable(world_clock_test world_clock_test.cpp) add_executable(world_units_test world_units_test.cpp) +add_executable(system_macro_headers_test system_macro_headers_test.cpp) add_executable(state_brick_frustum_test state_brick_frustum_test.cpp) add_executable(state_conflict_ring_test state_conflict_ring_test.cpp) add_executable(state_zstd_round_trip_test state_zstd_round_trip_test.cpp) @@ -222,6 +223,7 @@ set(TEST_TARGETS state_brick_manifest_test world_clock_test world_units_test + system_macro_headers_test state_data_hydrator_test state_node_telemetry_test state_telemetry_aggregator_test @@ -1337,6 +1339,13 @@ target_link_libraries(world_units_test GTest::gtest_main ) +target_link_libraries(system_macro_headers_test + PRIVATE + cvc + GTest::gtest + GTest::gtest_main +) + target_link_libraries(state_brick_frustum_test PRIVATE cvc @@ -1511,6 +1520,7 @@ target_compile_features(state_hash_partition_test PRIVATE cxx_std_17) target_compile_features(state_hybrid_time_test PRIVATE cxx_std_17) target_compile_features(world_clock_test PRIVATE cxx_std_17) target_compile_features(world_units_test PRIVATE cxx_std_17) +target_compile_features(system_macro_headers_test PRIVATE cxx_std_20) target_compile_features(state_brick_frustum_test PRIVATE cxx_std_17) target_compile_features(state_conflict_ring_test PRIVATE cxx_std_17) target_compile_features(state_zstd_round_trip_test PRIVATE cxx_std_17) @@ -1812,6 +1822,7 @@ cvc_discover_tests(state_hash_partition_test) cvc_discover_tests(state_hybrid_time_test) cvc_discover_tests(world_clock_test) cvc_discover_tests(world_units_test) +cvc_discover_tests(system_macro_headers_test) cvc_discover_tests(state_brick_frustum_test) cvc_discover_tests(state_conflict_ring_test) cvc_discover_tests(state_zstd_round_trip_test) diff --git a/src/cvc/tests/system_macro_headers_test.cpp b/src/cvc/tests/system_macro_headers_test.cpp new file mode 100644 index 000000000..c10dc3fea --- /dev/null +++ b/src/cvc/tests/system_macro_headers_test.cpp @@ -0,0 +1,87 @@ +/* + Copyright 2026 The University of Texas at Austin + + This file is part of libcvc. + + libcvc is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License version 2.1 as published by the Free Software Foundation. +*/ + +// Header-hygiene regression: libcvc's public headers must compile when a +// consumer includes system headers FIRST. Those headers define lowercase +// macros that silently rewrite any identifier of the same name, e.g. glibc's +// has `#define si_value _sifields._rt.si_sigval`, which turned the +// parameter in `double to_display(double si_value, dimension d) const;` into a +// syntax error for every TU that included before +// (seen in volrover3's dialogs against libcvc 3.5.0). +// The test is that this TU compiles; the TEST below only makes it a real +// executable that links and runs. +// +// The system headers stay in their own preprocessor block, ahead of the libcvc +// includes, so include sorting can never move them after the headers they guard. + +#ifndef _WIN32 +#include // si_value, si_pid, si_addr, ... (glibc/musl siginfo_t accessors) +#include // function-like MIN(a,b) / MAX(a,b) +#include // st_atime / st_mtime / st_ctime +#include // major / minor / makedev on macOS and older glibc +#else +// Same guards the library uses around (see src/cvc/image/magick_io.cpp). +#ifndef WIN32_LEAN_AND_MEAN +#define WIN32_LEAN_AND_MEAN +#endif +#ifndef NOMINMAX +#define NOMINMAX +#endif +#include // ERROR, DELETE, IN, OUT, OPTIONAL, near, far, RGB(), ... +#endif + +// A representative public header from every module in the cvc target. Not +// included: cvc/gl/* (the optional cvcGL library, needs VTK/OpenGL), +// cvc/volume/hdf5_utils.h (HDF5), cvc/geometry/project_verts.h (CGAL) and +// cvc/utility/cuda_utils.h / cvc/volren/raycaster_cuda.h (CUDA). +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +TEST(SystemMacroHeaders, WorldUnitsUsableAfterSystemHeaders) { + cvc::world_units u; + EXPECT_DOUBLE_EQ(u.to_display(1.5, cvc::world_units::dimension::length), 1.5); + const cvc::world_units::measurement m = u.format(2500.0, cvc::world_units::dimension::length); + EXPECT_DOUBLE_EQ(m.value, 2.5); + EXPECT_EQ(m.unit, "km"); +} + +TEST(SystemMacroHeaders, UtilityMinMaxUsableAfterSysParam) { + // may define function-like MIN/MAX macros in this TU, so call + // the cvc templates the macro-proof way. + EXPECT_EQ((cvc::MIN)(3, 5), 3); + EXPECT_EQ((cvc::MAX)(3, 5), 5); +}