Skip to content

build: add clang-tidy - #80

Merged
oberbichler merged 1 commit into
mainfrom
chore/clang-tidy
Jul 26, 2026
Merged

oberbichler merged 1 commit into
mainfrom
chore/clang-tidy

Conversation

@oberbichler

Copy link
Copy Markdown
Owner

State before

clang-format was already declared, in the dev group. Worth noting for expectations: it is not persistently in .venv/bin, because uv sync prunes non-default groups — uv run --only-group dev clang-format provisions it per call in milliseconds, which is also how the format CI job works.

clang-tidy was not available at all: not on PATH, no brew llvm, and the Xcode command line tools do not ship it.

Its own dependency group

dev  = ["clang-format>=19.0", "ruff>=0.11"]
tidy = ["clang-tidy>=19.0"]

Not in dev, because of size — measured in fresh isolated environments:

group wheel environment
dev (clang-format + ruff) 1.8 MB 26 MB
tidy (clang-tidy) 44 MB 115 MB

In dev, format.yml would pull those 44 MB on every run without using them. Installed system-wide via brew install llvm would have worked too, but the project convention is the uv group, and that one is reproducible and works in CI.

Two things were needed to make it run at all

A compilation database. just compile-db generates it from the test build, which is the right entry point: that is what instantiates the templates, and an uninstantiated template is never analysed. Same reason the warnings from #75 saw nothing in the header 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:

doctest.h:462:10: error: 'signal.h' file not found [clang-diagnostic-error]
Error while processing test_variants.cpp.

That left the unit half parsed, so the results were silently incomplete. CMAKE_OSX_SYSROOT puts the sysroot into the database; afterwards the unit parses with 0 errors.

Check selection is measured, not guessed

Running everything produced 750+ findings, dominated by two checks that fight the domain:

check findings why it is off
readability-identifier-length 427 i, j, f, g, da, daa are the notation
modernize-use-trailing-return-type 225 a style the project does not use

.clang-tidy turns the families on and names every exclusion with its reason. Two are worth revisiting rather than being settled:

  • modernize-use-nodiscard (57 sites) — annotating the API with [[nodiscard]] is a decision to make on purpose, not a lint fix. Arguably valuable for a math library, where ignoring the result of a + b is a bug.
  • bugprone-throwing-static-initialization (26) — only ever fires on the deliberate const DDScalar<2, double, 3> dd1{...} test fixtures; the header has no globals. Not nothing, though: if such a fixture ever threw during static init the binary would terminate with no diagnostic, which is exactly the failure mode fix: size the storage in the initializer list constructor #74 produced (exit 133, no output).

bugprone-branch-clone is off as a false positive: it flags the if constexpr in the initializer-list constructor as having "identical then and else branches", where they are m_data.assign(...) and std::copy(...).

Result: 51 findings, 0 errors

20  readability-braces-around-statements       all in SScalar
 7  modernize-avoid-c-style-cast
 4  readability-use-concise-preprocessor-directives
 4  misc-const-correctness
 3  readability-redundant-access-specifiers
 2  readability-redundant-typename
 2  modernize-type-traits
 2  modernize-pass-by-value
 2  misc-use-internal-linkage
 2  misc-non-private-member-variables-in-classes
 1  readability-else-after-return
 1  performance-unnecessary-value-param

Several land on open items from the review — the public m_size/m_data, hm(const std::string) by value, std::conditional<...>::type at lines 53 and 749 — and two land on code I added myself (the index(T) cast from #78, the make/check helpers from #79).

One finding is new, and it is the most interesting:

hyperjet.h:322: 1 uninitialized field at the end of the constructor call
                [clang-analyzer-optin.cplusplus.UninitializedObject]
  322 |   DDScalar(const Data &data) : m_data(data) {
   56 |   index m_size;   <- uninitialized

The static-only constructor leaves m_size indeterminate. size() returns TSize for static types so nothing reads it, but every copy of such an object copies an indeterminate value — formally UB, and MSan would flag it. Same family as the review's note about empty() copying an uninitialized std::array.

Verification

  • just tidy from scratch: exit 0, 51 warnings, 0 errors
  • just format and clang-format --Werror unchanged green
  • group separation confirmed in fresh isolated environments
  • just --list descriptions render correctly
  • no source changesgit status shows nothing under include/, test/ or python/src/

run-clang-tidy.py was tried and dropped: with two translation units it is no faster (1:56 against 1:58) and reports every header finding once per unit, 89 instead of 51.

Two things to decide

The justfile was untracked and is included here. The sysroot workaround and the compile-db step are not obvious enough to leave in a single working copy — but if you would rather keep that file local, say so and I will drop it from the PR and put the commands somewhere else.

No CI job. With 51 findings, --warnings-as-errors would be red immediately. My suggestion is to work through the ~10 substantive ones first and then add a job — separate from format.yml, so the cheap format check does not start pulling 44 MB.

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.
@oberbichler
oberbichler merged commit 5a2c871 into main Jul 26, 2026
17 checks passed
@oberbichler
oberbichler deleted the chore/clang-tidy branch July 26, 2026 21:39
oberbichler added a commit that referenced this pull request Jul 27, 2026
m_size only means anything when TSize is Dynamic. For every other variant it
was eight bytes that nothing reads, and the constructor taking Data never set
them, which is what clang-tidy reported in #80:

    hyperjet.h:322: 1 uninitialized field at the end of the constructor call
                    [clang-analyzer-optin.cplusplus.UninitializedObject]
      322 |   DDScalar(const Data &data) : m_data(data) {
       56 |   index m_size;   <- uninitialized

Nothing read the field, but every copy of such an object copied an
indeterminate value, which is UB to read and would trip MSan.

The field now has an empty stand-in type for the static variants, marked
[[no_unique_address]], so a static scalar is exactly its data:

                                  before   after
    DDScalar<1, double, 3>            40      32
    DDScalar<2, double, 3>            88      80
    DDScalar<2, double, 8>           368     360
    DDScalar<2, double, 16>         1232    1224
    DDScalar<2, double, Dynamic>      32      32

The initializer list constructor is the one place that ran for both variants
and set the field from its member initializer list, so it computes the size
into a local and only stores it when there is somewhere to store it.

This also settles the layout question a NumPy dtype would ask of the element
type: a static scalar is now payload-sized and trivially copyable, and the
dynamic one is neither, which is why only the static variants could ever back
a dtype.

Not a performance change, but measured rather than assumed. Five alternating
runs of `a * b + a.sin()` over 20k elements, -O3 -DNDEBUG: size 3 unchanged at
about 10.85 ns per element, size 8 consistently a little faster, 51.6-52.4
against 52.0-52.9. The first measurement suggested a 12 percent regression and
was cold-start noise.

  C++ 102 test cases, 1310 assertions, in Release with -Werror and under clang
  with ASan+UBSan
  Python 172 passed, exported API identical against a build of the previous
  state
  clang-tidy down to 50 findings, the uninitialized field gone
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant