Repository navigation
ci: add a sanitizer job and turn on compiler warnings - #75
Merged
Merged
Conversation
Three memory-safety bugs in a row were found by running the tests under
AddressSanitizer by hand. This makes that part of CI.
Two options on the test target, both off by default:
- HYPERJET_SANITIZE builds with -fsanitize=address,undefined and
-fno-sanitize-recover=all, so undefined behaviour aborts instead of
printing and passing.
- HYPERJET_WERROR promotes warnings to errors. Warnings themselves
(-Wall -Wextra -Wpedantic, /W4) are always on, so they are visible
locally too, but only one CI job fails on them — warning sets differ
between compilers, and an unrelated toolchain should not be able to
block every build.
The new job runs both on clang, which also covers a third front-end next
to GCC 14 and MSVC. Eigen and doctest are now included as SYSTEM headers so
third-party warnings cannot fail our build.
Verified that the job is not decorative: with a reproducer for the
initializer list bug still open in #74, it fails as intended —
test case CRASHED: SIGSEGV - Segmentation violation signal
==1413==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000
#7 DDScalar<2l, double, -1l>::DDScalar(std::initializer_list<double>)
hyperjet.h:314
Warnings that -Werror surfaced:
- SScalar::hypot kept f3, a2, b2 and c2 from the DDScalar version, where
they carry the second-order terms. SScalar is first order only, so they
were dead.
- DDScalar(const Data&, index) initialized m_data before m_size while the
declaration order is the reverse (-Wreorder-ctor). Harmless today, since
the vector constructor does not read m_size, but it is a warning inside
a header shipped to users.
Neither of those warnings appeared before, because nothing instantiated the
functions: the tests never touched SScalar::hypot or a dynamic DDScalar. A
warnings job is only as good as the instantiation coverage, so this adds the
missing SScalar::hypot test case — which is what surfaced the dead
variables. The dynamic constructor gets covered by the tests in #73.
Also adds <algorithm> and <stdexcept>, which the header used through
transitive includes only. Included here because the new job compiles with a
front-end the project has not built with before, and missing includes are
exactly what breaks first under a new toolchain.
Workflow-level permissions: contents: read while in this file. The other
three workflows still need the same.
This was referenced Jul 26, 2026
This was referenced Jul 26, 2026
oberbichler
added a commit
that referenced
this pull request
Jul 26, 2026
The C++ suite ran almost entirely on DDScalar<2, double, 3>. Templates that are never instantiated are never compiled, so neither the tests, nor the warnings, nor the sanitizer job from #75 could see the first-order or dynamic code paths at all. That is how the deduction failure in #78 survived, and why turning on -Wall -Wextra produced zero warnings over a header that had five. Adds a second layer in its own translation unit: 14 TEST_CASE_TEMPLATEs over {order 1, 2} x {static, dynamic}, covering accessors and the linear Hessian index, the factories, all arithmetic in both binary and in-place form with both operand orders, one function per arity (unary, binary, ternary), eval, and padding. 56 test cases, 853 assertions. The expected values were derived symbolically from quadratic Taylor polynomials, so they do not share formulas with the header. One set of numbers serves both orders: the first-order part of every operation depends only on the first-order parts of its inputs, so truncating a second-order expectation to [f, g...] gives the first-order one -- the same trick the Python tests use. These pass on the first run, which on its own proves nothing, so they were validated by mutation. Two deliberate faults, counting failures in the old suite against the new one: unary() skips the gradient loop at order 1 old 0 new 4 data_length() one short for dynamic old 1 new 22 The first is invisible to the old suite. It would break every math function for every first-order type and still ship green. Kept separate from test.cpp rather than converting it: that file holds hand-verified numbers for one variant in full detail, which is worth keeping as-is, and a separate TU compiles in parallel (1.8 s next to 3.2 s). No production changes. Verified in Debug, Release and under clang with ASan+UBSan, all with -Werror: 101 test cases, 1302 assertions. Python 172 passed, unchanged with the file stashed.
Merged
oberbichler
added a commit
that referenced
this pull request
Jul 26, 2026
clang-format was already declared, in the dev group. clang-tidy was not available at all: not on PATH, no brew llvm, and the Xcode command line tools do not ship it. It goes into a group of its own rather than into dev, because the wheel is 44 MB against 1.8 MB for clang-format (115 MB against 26 MB unpacked, measured in fresh isolated environments). In dev, the format CI job would pull it on every run without using it. Two things were needed to make it actually run on this project: A compilation database. The test build is the right entry point, since that is what instantiates the templates -- an uninstantiated template is never analysed, which is the same reason the warnings from #75 saw nothing until #79 added the missing instantiations. The macOS SDK. CMake omits -isysroot because AppleClang finds the SDK implicitly; the standalone clang-tidy does not, and failed on signal.h from doctest, leaving the translation unit half parsed. CMAKE_OSX_SYSROOT puts the sysroot into the database, after which the unit parses with zero errors. The check selection is measured rather than guessed. Everything on produced 750+ findings, dominated by readability-identifier-length (427 -- i, j, da and daa are the notation of the domain) and modernize-use-trailing-return-type (225 -- a style the project does not use). The config turns the families on and names each exclusion with its reason, including two that are worth revisiting: modernize-use-nodiscard is off because annotating 57 members is an API decision rather than a lint fix, and bugprone-throwing-static-initialization only ever fires on the deliberate test fixtures. What remains is 51 findings and no errors. run-clang-tidy.py was tried and dropped: with two translation units it is no faster (1:56 against 1:58) and reports every finding in the header once per unit, 89 instead of 51. The justfile was untracked until now. It is included because the sysroot workaround and the compilation database step are not obvious enough to leave in one working copy. No source changes.
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
1 participant
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.
Why
Three memory-safety bugs in a row (#72, #73, #74) were found by running the tests under AddressSanitizer by hand. This makes that part of CI.
What
Two options on the test target, both off by default:
HYPERJET_SANITIZE-fsanitize=address,undefined -fno-sanitize-recover=all -fno-omit-frame-pointer— undefined behaviour aborts instead of printing and passingHYPERJET_WERRORWarnings themselves (
-Wall -Wextra -Wpedantic,/W4) are always on, so they show up locally too. Only the one new job fails on them: warning sets differ between compilers, and an unrelated toolchain should not be able to block every build.The job runs both on clang, which also covers a third front-end next to GCC 14 and MSVC. Eigen and doctest are now included as
SYSTEMheaders so third-party warnings cannot fail our build. Addedpermissions: contents: readwhile in this file.The job is not decorative
Verified against a reproducer for the initializer-list bug that #74 fixes, which is still present on
main:Exit code 134, pointing at the exact line. The reproducer was removed again — it belongs to #74.
Warnings this surfaced
SScalar::hypotkeptf3,a2,b2,c2from theDDScalarversion, where they carry the second-order terms.SScalaris first order only, so they were dead code.DDScalar(const Data&, index)initializedm_databeforem_sizewhile the declaration order is the reverse (-Wreorder-ctor). Harmless today, because the vector constructor does not readm_size— but it is a warning inside a header shipped to users.Neither warning appeared before this PR, and the reason matters: nothing instantiated those functions. The suite never touched
SScalar::hypotor a dynamicDDScalar, and templates that are not instantiated are not compiled. A warnings job is only as good as the instantiation coverage.So this also adds the missing
TEST_CASE("SScalar Hypot")— genuinely absent coverage (both the 2- and 3-argument form), and what surfaced the dead variables in the first place. The dynamic constructor gets its coverage from the tests in #73; that is also why #73 would have failed a-Werrorbuild before the reorder fix in this PR.Also included
<algorithm>(std::copy,std::fill) and<stdexcept>(std::runtime_error), which the header only ever got through transitive includes. Two lines, deliberately in scope: the new job compiles with a front-end the project has not built with before, and missing includes are exactly what breaks first under a new toolchain — I did not want the job's first result to be a failure for an unrelated reason.Verification
Ran the exact command chain the job runs, locally with clang:
-Werrorbuild clean in Debug and Release, 41 test cases / 410 assertionsclang-format --WerrorcleanNot verifiable locally: the GCC 14 and MSVC warning sets (macOS has only Apple clang here). That is precisely why
-Werroris scoped to the one clang job rather than the whole matrix.Notes
Independent of #73 and #74 — this branch is based on
mainand merges in any order. Note that after #73 and #74 merge, the new job covers strictly more, since both add dynamic-type tests.Follow-ups from the review that this PR does not do:
permissions: contents: readfor the other three workflows.{order 1, 2} × {static, dynamic}C++ test axis — the single biggest lever on what the sanitizer and warnings jobs can actually see.