Dead code, de-duplication, repo-wide clang-format, and lint gates in CI

[ZacharyZcR]

Draft: opening it early so the direction can be checked before the details are argued over.

Six commits: dead code, de-duplication, a repo-wide clang-format pass, the two suppressed warnings, and lint gates in CI (clang-format, ruff, clang-tidy). Each commit stands alone and explains itself; this is the summary.

The part that is not cleanup: four real defects

These came out of the lint work rather than from looking for them.

Three unchecked reallocs whose next statement dereferences the result (3439a8d). p = realloc(p, n) returns NULL on failure, p becomes NULL with it, and the store that follows is a null dereference — not the clean exit-with-a-message every other allocation on these paths produces, and the original buffer leaks on the way out.

• json.h j_parse, object growth (keys + kids)
• json.h j_parse, array growth (kids)
• st.h, the format-stamp table (fmt_name + fmt_val)

Two of the three are in the JSON parser — the code that reads config.json, which st.h's own comments call an untrusted container.

A snprintf truncation that silently retargets a file write (cf72f78). route_trace.h rt_save builds "<path>.tmp" into a 2100-byte buffer and then does write-then-rename. A longer path truncates, and the rename lands somewhere the caller never asked for. It now refuses and says so.

dev does not currently build warning-free

make kimi_k3 on dev emits five warnings, and CONTRIBUTING says a PR is reviewed for "a clean build (0 warnings)". None of them are suppressed by the -Wno- flags in CFLAGS — make output just scrolls past, and only a failed build stops CI. Fixed in cf72f78; the snprintf one above was hiding in there.

Dead code: less than expected, and my first pass missed some

Eight functions, ~100 lines. Worth saying how they were found, because the obvious method does not work: grepping for a name is defeated by prose (attention appears 44 times in comments and 0 times as a call). -Wunused-function is what actually found them — intersected across all four engines, since a header function only one engine calls is "unused" in the other three.

uring_wait_all, unpack_rows, e8_pow2_ceil, stops_arm, attention, cmp_fdesc, st_prefetch. st_prefetch_rep, st_mirror_init and st_read_raw_cap look dead from inside one engine but are used by the tests, so they stay.

One of them was not dead, and the fix for that is in 6b4721b. rt_tracing is called by colibri.c's device-side router under #ifdef COLI_CUDA, so a default CPU build sees no caller while the CUDA build stops compiling. The intersection-over-four-engines method only ever looked at the default configuration; a reference from inside any #ifdef COLI_* arm is invisible to it. On top of that, implicit-function-declaration is a warning on gcc <= 13 and an error on gcc >= 14, so locally it printed a warning among the build output and the engine linked anyway.

Rather than just putting the function back, make -C c check-configs now parses all four engines under all eight combinations of the optional backends (CUDA, Vulkan, Metal, ANS), with -Werror=implicit-function-declaration. It is -fsyntax-only — about a second, no backend SDK needed — where until now the only job able to notice was the five-minute Windows CUDA build. Verified the gate works by deleting rt_tracing again: it fails on the three COLI_CUDA configurations, and passes once restored. The function now carries a comment saying why it looks unused, so the next dead-code report does not claim it again.

De-duplication: three places, and three deliberate refusals

Taken (a68d459):

• vk_spv.h (new) — shader-path resolution was byte-identical in colibri.c vk_resolve_spv and kimi_k3.c k3_vk_spv, so [Performance]: Vulkan (#418) vs ROCm/HIP on RDNA4 (RX 9070 XT) — CORRECTED: Vulkan is 19–24% faster (original comparison was confounded) #523's "COLI_VK_SHADERS may be a directory" fix had to be made twice.
• tok.h — km_letters was o2_letters with Han masked out of S1/S2: the same 31 lines of regex-backtracking replay, twice. Likewise pretok_chunk_kimi was pretok_chunk_o200k plus rule H and minus rule D's / tail, so rules C/E/F/G lived in two copies that are supposed to stay identical. cl100k's splitter is left alone — it differs structurally, and folding it in would produce a three-way branch, not shared code.
• backend_vulkan.c — coli_vk_mem_budget2 duplicated the whole VK_EXT_memory_budget query for G2.

Refused, because sharing them would produce worse code than the duplication:

• arena_suballoc_d2 differs in device, memtype, arena head and memory-priority support — sharing needs a six-argument function.
• attention_absorb_project is absorb followed by an output projection pass, not a copy of absorb.
• rans_kernel_scalar_bf is a deliberate branch-free twin, kept to validate the SIMD arms' algebra. Merging it would destroy the reason it exists.

clang-format (003a0b9, format-only)

.clang-format has been in the repo from the start and nothing ran it, so all 88 files had drifted.

Not reformatted, on purpose: *.mm has no automated coverage at all (the macOS job builds the default CPU target, which does not compile backend_metal.mm) and *.cu needs nvcc, which only CI has. Reformatting a file nothing can check is how a format pass turns into a silent regression. The CI check skips them for the same reason; they can follow once there is a gate that would catch it.

One block is fenced with clang-format off. CKR expands to a braced if with no trailing semicolon, so clang-format reads each CKR as the body of the one before it and indents the config-validation table one level deeper on every run. It never reaches a fixed point — the format check would have failed on a tree that had just been formatted — and it turned a tidy two-column table into a staircase. Formatting is otherwise idempotent.

Equivalence is demonstrated, not asserted. The preprocessed token stream of all four engines was diffed before and after. The only differences are four long string literals that clang-format split into adjacent literals, which translation phase 6 concatenates back into the identical string. Everything else is token-for-token identical.

The two tok.h rewrites got their own cross-check against the previous code: 13.2M o2_letters/km_letters calls and 900k pretok splits across all three tokenizer families, zero divergence.

.git-blame-ignore-revs is included; enable it with git config blame.ignoreRevsFile .git-blame-ignore-revs (GitHub honours it automatically).

Warnings and lint (20b6143, 3439a8d)

Two of the three -Wno- flags are gone. -Wmisleading-indentation costs nothing now — reformatting removed all 18 — and it is the warning that catches if (c) a; b; where b was meant to be guarded. -Wunused-parameter had one hit.

-Wunused-function stays off, and $(WARN_OFF) records why in full. The engines are single translation units including header-only libraries, so a header function only one engine calls is "unused" in the other three: 62 warnings that cannot be acted on, because there is no other .o for the compiler to see the caller in.

ruff (pyproject.toml) selects pyflakes plus ruff's default pycodestyle subset, and disables the four rules that only disagree with how this code is written — E701/E702/E401 (the compact several-statements-per-line style the C side uses too), E741 (l is the layer index, I/O the matmul dims, matching the C and the papers) and E402 (imports inside the branch that needs them, so a tool starts without torch installed). That left 55 real findings, all fixed.

.clang-tidy is tuned to zero findings on the current tree. The full bugprone-* set produces 434 findings across the four engines and, having gone through them by category, not one is a defect — they are narrowing conversions in index arithmetic that is explicitly widened exactly where width matters, if (!strcmp()), if (!(p = malloc(n))), _GNU_SOURCE, and the (S, D, I, O) dimension quadruple. Each of the ten disabled checks has its reason recorded in the file. A check that starts life with hundreds of pre-existing hits is a check nobody reads. bugprone-suspicious-realloc-usage is not disabled — it is the one that found the three defects above, and it stays on to catch the next one.

clang-analyzer-* is deliberately not enabled yet. It reports path-sensitive findings (allocation size 0, potential leak, nonnull violation) that each need tracing by hand to tell a real defect from an invariant the analyzer cannot see. Worth doing — as its own change, not smuggled in behind a lint config.

New targets, none of which need a model, a GPU or a build: make -C c lint (clang-format + ruff, seconds), make -C c check-configs (the backend matrix above, about a second) and make -C c lint-tidy (separate, because it is a full compile plus analysis per engine). Two CI jobs to match; lint is the cheapest job in the file, so it reports first.

lint-c warns when clang-format's major version is not the 18 that CI pins — its output is not stable across majors, and an unpinned check would fail PRs over the contributor's distro rather than their code.

Verification

All four engines build warning-free on gcc and clang, and parse under all eight backend configurations (make -C c check-configs). make test-c passes, test_tok_o200k is 40/40 encode + decode, the Python suite is 288 tests OK (18 skipped), and lint-c / lint-py / lint-tidy are all clean. CI is green on all 15 jobs, including the Windows MSVC CUDA build.

Not verified here, and I would rather say so than imply otherwise: no GPU path was run (no CUDA/Metal/Vulkan device available here) — the backend configurations are compile-checked, not executed. *.mm and *.cu are untouched.

One pre-existing warning is left alone: make colibri CUDA_DLL=1 reports g_cuda_raw_experts defined but not used, because its only reader sits under #ifdef COLI_ANS. That is on dev today and is not something this PR introduced, so it is mentioned rather than changed.

On reviewing this

003a0b9 is pure formatting and completely independent of the other four — if the diff size is awkward it can be taken separately, before or after, without affecting them. The other four are each small and self-contained.

[JustVugg]

Keeping this one for last deliberately, and I want to say why rather than let it sit.

A repo-wide clang-format is not a normal PR when the queue is this deep. +25,371 / −16,130 across 107 files touches almost every region every other open PR touches. There are around fifty open PRs right now; merging this puts most of them in conflict on the same day, including work that cannot absorb it — #377 and its split (56,750 lines), #165's follow-ups, #729 (11,788 lines). Some of those authors will not come back for a second rebase, and the cost of that is not paid by us.

So: this lands when the queue is short, and it gets announced before it does. Not a judgement on the change — the dead-code removal and the lint gates are worth having, and I would rather have them than not.

It now conflicts with dev since #165 landed. 14 hunks, and unlike most of today's conflicts these are not additive:

c/st.h                    6
c/tok.h                   3
c/olmoe.c                 2
c/tests/test_pipe_block.c 1
c/tests/test_st_pread.c   1
c/tests/test_uring.c      1

st.h and tok.h are the two files #165 also reworked, so those six and three hunks are formatting-vs-substance collisions rather than two sides adding lines. I resolved the additive conflicts on other branches myself and pushed them; I am not touching these, because a reformat that silently alters a st.h code path is exactly the failure this PR is otherwise designed to prevent.

My suggestion, and it is only that: rebase it after the queue drains rather than now. Reformatting a moving target means doing this again, and each pass burns the same fifty PRs.

[ZacharyZcR]

Agreed, and I would rather it wait than land on the queue you are describing.

The arithmetic is not arguable: +25,371 / −16,130 across 107 files touches almost every region every other open PR touches. Fifty authors rebasing on the same day so my formatting PR can go in first is a cost I would be imposing on people who get nothing out of it — and #377's 56,750 lines and #729's 11,788 are exactly the branches least able to absorb it. If one of those authors does not come back, that is a much worse outcome than a repo that stays unformatted for another few weeks.

I will keep it as-is and rebase when you say the queue is short. Rebasing now is chasing a moving target: every merge invalidates the reformat, and each pass costs the same fifty PRs.

On the 14 conflicts — do not resolve these, and thank you for not trying. Your reason is the right one and it is the same argument the PR makes: a reformat that silently alters a code path in st.h is precisely the failure this change exists to prevent. st.h (6) and tok.h (3) are the two files #165 reworked, so those are formatting-vs-substance collisions, and the only safe resolution is to re-run clang-format on the post-#165 tree rather than to merge hunks by hand. That is a mechanical operation once, at the right moment — not nine judgement calls now.

Two things I would ask when the window opens:

1. Announce it before, not with. A day's notice on an issue lets anyone mid-rebase land or pause first. I am happy to write that notice.
2. Split it if you would rather. The dead-code removal and the lint gates in CI are the parts with actual value, and they are small and reviewable on their own. The repo-wide clang-format is the part that costs everyone. If you want the first two now and the reformat later, I will separate them — that is a better trade than holding all three hostage to a quiet queue.

Say which you prefer and I will do that instead of waiting.

[JustVugg]

Closing on the conclusion we already reached together in this thread: a 107-file, +25k/-16k format pass cannot be mergeable while the queue is this deep — every merge invalidates it, and it has now been stale for almost two weeks against a fast-moving dev.

The right shape is two PRs, and I'd take both:

1. The lint gates alone — small, mergeable today, and they're the part that stops the problem recurring.
2. The format pass — timed to a deliberately quiet window, so it lands in one shot instead of racing the queue.

The dead-code and de-duplication findings are the most valuable part and shouldn't wait on formatting at all; if you split those out, they can go in independently. Reopening isn't necessary — a fresh PR per slice is cleaner.

[monotophic]

Authored by Claude Opus 5 in Claude Code, analysis in partnership with
@monotophic.

Following up on the three unchecked realloc fixes in here, because we have a
concrete stake in seeing them land.

Why we care. We are building a faithful FP8 checkpoint container — a repack
that stamps o_proj across 79 layers and adds a container-level format
declaration. That makes c/st.h:380 load-bearing for a new format's
correctness: it is the growth path of the format-stamp table, and a stamp lost
there does not fail loudly. The tensor simply reads as unstamped, the reader
falls back to byte-arithmetic format inference, and at the ambiguous shape that
is exactly the case that cannot be resolved by arithmetic. So for us these are
correctness-critical rather than hygiene.

#	site (dev)	what happens
D1	c/st.h:380-384	fmt_name/fmt_val take the realloc result directly; on failure the pointer becomes NULL, the original block leaks, and the next statement dereferences it
D2	c/json.h:119	object growth in j_parse, same pattern on v->keys/v->kids
D3	c/json.h:137	array growth in j_parse, same pattern on v->kids

Two things make these defects rather than style preferences. The file disagrees
with itself — c/json.h:66 (J_PUT) handles the same situation correctly a few
dozen lines earlier, checking and exiting with a message. And these paths are
already reasoned about as untrusted: st.h carries a
"refusing (untrusted container)" exit just above D1, and its comments treat
config.json as untrusted input, which is exactly what j_parse consumes.

We reproduced the failure rather than relying on inspection. With an interposed
allocator returning NULL at each growth point, the unpatched headers take
SIGSEGV at exactly the predicted NULL-base store; with your checks in place the
same cases exit cleanly with a diagnostic.

One addition, which your patch does not cover. At c/st.h:383-384 on dev,
two lines below the stamp-table realloc you fix:

S->fmt_name[S->fmt_n] = strdup(inner->keys[i]);
S->fmt_val[S->fmt_n]  = strdup(v->str);

Failing the first gives the same crash class as the original defect, in the next
iteration's duplicate-scan strcmp. Failing the second is quieter and worse:
the process survives with exit 0 and no diagnostic, st_fmt_stamp() returns
NULL for that tensor, and the tensor reads as unstamped — so a single failed
allocation silently disables a trust-verify-refuse check instead of refusing.
These run twice per stamp entry, so they are the frequent allocation on that
path; the realloc is the rare one. That is two more lines in the same hunk if
you want it, or we can bring it separately — whichever suits.

On frequency, for completeness rather than as an argument: our change adds
roughly 1,700 realloc calls at c/json.h:119 per model load and two at
c/st.h:380-381, and none at c/json.h:137 — the longest JSON array in a
shard header is 2 elements against a starting capacity of 8. Existing int4
containers already produce headers of comparable size, so engine exposure is
not materially different. The reason we are here is the correctness point
above, not the call count.

What we would like to see. Your patch for these three sites is already
correct, and we would like it to land with your authorship @ZacharyZcR.
The thread has settled that this PR gets split; we have no view on where these three
fixes belong in that split, and we are not asking you to reshape it around us.
What we are asking is simply that they land as part of a bug-fix PR — bundled
with whatever else fits, or targeted narrowly at these sites, whichever you
prefer.

We are glad to help in whatever way is actually useful:

• we will test and support the PR once it is up, on both CPU and CUDA paths;
• we can contribute the reproduction harness to it;
• if you would rather we carried it, we can post the bug-fix PR and credit your
  find and authorship @ZacharyZcR.

On the rest of #798. We would like to see this work land in some form. Dead
code removal, de-duplication, a repo-wide format pass and lint gates in CI are
the kind of changes that keep paying out long after they merge — and the defect
findings above came out of exactly that work, which is a concrete argument for
its value rather than a general one. It was set aside for shape and queue depth,
not for being wrong. Whatever slices come back, we are happy to review and test
them as they go up, and to say so on the threads where that helps.

[monotophic]

@ZacharyZcR the bug-fix described in my post above is now PR #1101. Thanks again for this work. I hope it lands eventually.