fix(math): make fast_math.hpp self-contained (missing <algorithm>) - #717
Merged
Conversation
fast_math.hpp uses std::find_if (in the piecewise-linear interpolation helper) but never included <algorithm>, so it only compiled when a prior include pulled <algorithm> in transitively. Including fast_math.hpp first (or on a toolchain with a leaner <cmath>) failed with 'no member named find_if in namespace std'. Add the include so the header is self-contained. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
✅Static analysis result - no issues found! ✅ |
finger563
added a commit
that referenced
this pull request
Aug 16, 2026
Now that fast_math.hpp is self-contained (#717), include it in the consumer so compiling it requires _USE_MATH_DEFINES on MSVC (it uses M_PI in non-template code). Run the find_package consumer across an OS matrix (ubuntu / macos / windows) so the windows leg actually verifies the exported espp::espp target propagates _USE_MATH_DEFINES. FetchContent + CPM stay Linux-only (OS-independent CMake behaviour). Consumer is run via ctest so the multi-config MSVC output layout needs no path hard-coding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes components/math/include/fast_math.hpp a self-contained header by explicitly including <algorithm>, removing reliance on transitive includes for std::find_if. This aligns with the “include what you use” expectation and fixes consumer builds where fast_math.hpp is included before other headers (as surfaced by the consumer smoke test in #715).
Changes:
- Add
#include <algorithm>to supportstd::find_ifused inpiecewise_linear().
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
finger563
added a commit
that referenced
this pull request
Aug 16, 2026
…ckage / FetchContent / CPM) (#715) * feat(lib): add CMake install/export for host C++ library (find_package + FetchContent) Give the cross-platform host C++ library a proper, modern CMake package so a separate project can consume it two ways with the SAME link name espp::espp: - find_package(espp REQUIRED) from an installed tree, and - FetchContent / CPM add_subdirectory(lib) from the build tree. Changes: - espp_pc gets target-based usage requirements: every include dir espp.cmake collects is exposed as $<BUILD_INTERFACE:...>, plus $<INSTALL_INTERFACE:include>, and cxx_std_23 + system link deps (pthread / ws2_32,winmm,iphlpapi) are PUBLIC so they propagate to consumers. - Namespaced ALIAS espp::espp for build-tree consumers, and EXPORT_NAME espp so the installed/exported target is also espp::espp (archive stays libespp_pc.a). - New espp_install_cmake_package() (in espp.cmake) installs the target via EXPORT esppTargets, the public headers (merged flat into <prefix>/include, mirroring lib/pc/include), esppTargets.cmake, and generated esppConfig.cmake / esppConfigVersion.cmake (SameMajorVersion) under lib/cmake/espp. All third-party deps are vendored (headers installed + objects in the .a); the only non-bundled PUBLIC dep is Threads, resolved via find_dependency(Threads) in the config. - Gate the python bindings behind option ESPP_BUILD_PYTHON (default OFF) so a plain `cmake -S lib` yields just the C++ lib + install/export with no pybind11 dependency. build.sh / build.ps1 pass -DESPP_BUILD_PYTHON=ON to preserve their behavior (this is what build_libraries CI publishes to lib/pc); the scikit-build wheel path (SKBUILD) is unaffected. The legacy lib/pc install (used by the pc/ test build and CI artifacts) is kept unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(lib): gate standard find_package install behind ESPP_INSTALL so build.sh doesn't write /usr/local The build_libraries CI job (build_linux + build_macos) failed because lib/build.sh runs `cmake --build . --target install` with no CMAKE_INSTALL_PREFIX. The newly added standard install/export (espp_install_cmake_package) therefore fired during the plain build.sh install and tried to write the export into the default system prefix (/usr/local), exiting 2. - Add option(ESPP_INSTALL ... OFF). The standard install/export (find_package(espp) / espp::espp) now only runs when ESPP_INSTALL=ON. build.sh leaves it OFF, so it installs ONLY the legacy lib/pc artifacts and never touches /usr/local. A real consumer opts in with `-DESPP_INSTALL=ON -DCMAKE_INSTALL_PREFIX=<prefix>`. - Guard the legacy lib/pc install (which writes into the espp SOURCE tree) behind `if(PROJECT_IS_TOP_LEVEL OR ESPP_BUILD_PYTHON)` so a FetchContent/CPM consumer's `cmake --install` no longer mutates the espp source tree. Bump cmake_minimum_required to 3.21 for a reliable PROJECT_IS_TOP_LEVEL. - Fix the misleading top-of-file status message: report C++-only by default and only mention Python bindings when SKBUILD/ESPP_BUILD_PYTHON is set. Verified: build.sh succeeds with zero /usr/local install lines and populates lib/pc; `-DESPP_INSTALL=ON` installs a find_package-able package that a separate consumer configures, builds, links (espp::espp), and runs against. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(lib): derive CMake version from git tag; pc uses find_package(espp); remove legacy lib/pc install Version: derive project(espp VERSION) from `git describe --tags` at configure time (strip leading 'v', keep MAJOR.MINOR.PATCH), falling back to 0.0.0 for tarball/no-git builds. Flows into esppConfigVersion.cmake via PROJECT_VERSION. The python wheel's version still comes from setuptools_scm (unchanged). Legacy lib/pc removed: drop the source-tree install (install(TARGETS ... ARCHIVE DESTINATION .../pc) + espp_install_includes) and the espp_install_includes helper. The standard install/export (espp_install_cmake_package) is now THE install path; ESPP_INSTALL defaults ON for a top-level build and OFF for a subproject. espp_install_python_module installs the python package under the standard prefix (<prefix>/espp) instead of lib/pc. pc/ consumes the installed package: pc/CMakeLists.txt now find_package(espp) + links espp::espp (with $<LINK_LIBRARY:WHOLE_ARCHIVE,espp::espp> to preserve whole-archive linking so global-ctor/registration code, e.g. the Windows timer-period adjustment, is not stripped). The RTPS limits/fragmentation config (RTPS_CONFIG_HEADER / RTPS_ENABLE_FRAGMENTATION / RTPS_MAX_SAMPLE_SIZE) is now exported as PUBLIC compile definitions on espp::espp so find_package consumers compile the rtps headers with the same ABI as the archive. Build scripts / CI / interop: lib/build.{sh,ps1} install to a gitignored <repo>/install staging prefix (ESPP_INSTALL=ON, ESPP_BUILD_PYTHON=ON); pc/build.{sh,ps1} configure with CMAKE_PREFIX_PATH=<repo>/install; build_libraries.yml installs to and uploads that prefix; run_interop.sh installs lib to /tmp/espp/install and points pc at it. .gitignore: lib/pc -> /install/. Docs (lib/README.md, pc/README.md, interop README) updated for the new flow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(lib): scope SKBUILD wheel path to espp's own top-level build Address PR #715 review: SKBUILD is configure-wide, so it is also set when a downstream scikit-build-core project pulls espp in via FetchContent. In that case espp is not the top-level project, and the bare if(SKBUILD) branch would build only the _espp wheel module and never create the advertised espp::espp target. Gate the wheel-only path on (SKBUILD AND PROJECT_IS_TOP_LEVEL) via a new ESPP_WHEEL_BUILD variable used by all three SKBUILD-specific checks, so a FetchContent/CPM consumer still gets the C++ library + espp::espp. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(lib): document C++ find_package/FetchContent consumption in root README; clarify header-install intent Review pass over the CMake packaging work: - Root README: add an explicit 'From C++' subsection alongside the Python one - find_package(espp) + espp::espp, and a FetchContent/CPM snippet - so the C++ consumer story is discoverable from the top level (not only lib/README). - lib/espp.cmake: clarify why ESPP_EXTERNAL_INCLUDES_SEPARATE is installed flat like the others (those dirs are on the build -I path and their headers are referenced flat, e.g. "magic_enum.hpp"/"hid/..."; a pure find_package consumer needs flat, unlike the old lib/pc helper). No functional change. - lib/README: correct the stale pip prerequisite (CMake >= 3.21, C++23 compiler). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(lib): propagate _USE_MATH_DEFINES to consumers on MSVC Address PR #715 review: espp's public headers (math/fast_math.hpp, several filter headers) use M_PI/M_PI_2, which MSVC's <cmath> only defines when _USE_MATH_DEFINES is set before the include. The exported espp::espp target did not carry it, so find_package / FetchContent consumers on MSVC (and clang-cl) would fail to compile those headers. Add it as a PUBLIC compile definition on the target under MSVC so it exports into esppTargets.cmake as an INTERFACE requirement. (The pc tests previously masked this by defining the macro themselves.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: add CMake consumer smoke test (find_package / FetchContent / CPM) Keep the three external-consumer paths for the cross-platform C++ library green: - lib/tests/consumer: a minimal parameterized consumer (one main.cpp + a CMakeLists that obtains espp via find_package, FetchContent, or CPM) linking espp::espp. - .github/workflows/cmake_consumer.yml: installs espp to a prefix, then builds + runs the consumer all three ways (FetchContent/CPM consume the checkout via SOURCE_DIR/CPM_espp_SOURCE, so no second clone). Least-privilege permissions + PR-number concurrency + draft guard, matching the other build workflows. - lib/README: document that FetchContent/CPM recurse the vendored submodules automatically, add a FetchContent snippet, and point at the CI smoke test. Verified locally: all three methods build + run against this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: exercise MSVC in the consumer smoke test (fast_math.hpp / M_PI) Now that fast_math.hpp is self-contained (#717), include it in the consumer so compiling it requires _USE_MATH_DEFINES on MSVC (it uses M_PI in non-template code). Run the find_package consumer across an OS matrix (ubuntu / macos / windows) so the windows leg actually verifies the exported espp::espp target propagates _USE_MATH_DEFINES. FetchContent + CPM stay Linux-only (OS-independent CMake behaviour). Consumer is run via ctest so the multi-config MSVC output layout needs no path hard-coding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(lib): propagate /utf-8 to MSVC consumers (fmt Unicode static_assert) The consumer smoke test's windows leg failed compiling espp's public headers: fmt/base.h: error C2338: 'Unicode support requires compiling with /utf-8' The vendored fmt (pulled in via logger.hpp) static_asserts /utf-8 on MSVC. The lib set /utf-8 only for its own build (directory-level add_compile_options), which is not part of the exported INTERFACE, so find_package / FetchContent consumers didn't get it. Add /utf-8 as a PUBLIC compile option on espp::espp (alongside the _USE_MATH_DEFINES definition) under MSVC. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: show a CPM snippet alongside FetchContent (root + lib README) Both consumption snippets side by side so CPM and FetchContent users can copy the right config directly, rather than only FetchContent with CPM mentioned in prose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
components/math/include/fast_math.hppusesstd::find_if(in the piecewise-linear interpolation helper) but never#include <algorithm>. It compiled only when some earlier include pulled<algorithm>in transitively; includingfast_math.hppfirst fails:One-line fix: add
#include <algorithm>so the header is self-contained. (std::find_ifis its only<algorithm>use.)Surfaced by the CMake consumer smoke test in #715 when the consumer included
fast_math.hppbeforelogger.hpp.Verified: a TU that includes only
fast_math.hppnow compiles (-std=c++23 -Icomponents/math/include).🤖 Generated with Claude Code