Dead code, de-duplication, repo-wide clang-format, and lint gates in CI - #798
Open
ZacharyZcR wants to merge 7 commits into
Open
Dead code, de-duplication, repo-wide clang-format, and lint gates in CI#798ZacharyZcR wants to merge 7 commits into
ZacharyZcR wants to merge 7 commits into
Conversation
Dead code (zero references anywhere in the tree, including tests, tools and every #ifdef arm): colibri.c uring_wait_all, inkling.c unpack_rows, quant.h e8_pow2_ceil, sample.h stops_arm. De-duplication, three places where the same logic was maintained twice: vk_spv.h (new): shader-path resolution was byte-identical in colibri.c (vk_resolve_spv) and kimi_k3.c (k3_vk_spv), so JustVugg#523's "COLI_VK_SHADERS may be a directory" fix had to be applied to both. One copy now. tok.h: km_letters was o2_letters with Han masked out of S1/S2 -- the same 31 lines of regex-backtracking replay, twice. It becomes o2_letters_masked(..., mask_han). pretok_chunk_kimi was likewise pretok_chunk_o200k plus rule H, minus rule D's '/' tail, so rules C/E/F/G were maintained in two copies that are meant to stay identical; both now call pretok_chunk_o2fam(..., kimi). 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; both entry points now call vk_mem_budget_of(phys, has_budget, ...). Not de-duplicated on purpose: arena_suballoc_d2 differs in dev, memtype, arena head and memory-priority support (sharing it needs a six-argument function, which reads worse than the copy); attention_absorb_project is absorb followed by an output projection pass, not a copy of it; and rans_kernel_scalar_bf is a deliberate branch-free twin kept to validate the SIMD arms' algebra. Verified: all four engines build clean on gcc and clang; colibri.c, kimi_k3.c and backend_vulkan.c also compile with -DCOLI_VULKAN; make test-c passes; test_tok_o200k 40/40 encode + decode. The two tokenizer rewrites were additionally cross-checked against the previous code -- 13.2M o2_letters/km_letters calls and 900k pretok splits across all three families, zero divergence.
`make kimi_k3` on dev emits five warnings, which CONTRIBUTING says a PR is
reviewed for not having ("a clean build (0 warnings)"). None of them are
suppressed by the -Wno- flags in CFLAGS, so they are simply escaping notice
-- `make` output scrolls past and only a failed build stops CI.
g_idot / g_i4s / g_xexp moved from quant.h to colibri.c. quant.h never
reads them; they gate call sites in colibri.c alone. Defining them in the
header meant every other engine that includes it built three unused
statics.
g_k3_vk moved inside kimi_k3.c's `#ifdef COLI_VULKAN`, where all ten of
its uses already are.
route_trace.h rt_save: check snprintf for truncation. This is not a
cosmetic silencing -- a path longer than 2095 chars produced a truncated
temp name, and the write-then-rename below would then land on a path the
caller never asked for. It now refuses and says so.
Four more dead functions, found by -Wunused-function rather than by
grepping: colibri.c `attention` (a wrapper around attention_rows with no
callers left) and `cmp_fdesc`, route_trace.h `rt_tracing`, st.h
`st_prefetch`. All four are unused in every engine and in tests/ and
tools/. st_prefetch_rep, st_mirror_init and st_read_raw_cap look dead from
inside a single engine but are not -- the tests use them -- so they stay.
All four engines build warning-free again on gcc; make test-c passes.
Format only -- no behavioural change. .clang-format has been in the repo
since the beginning but nothing ran it, so all 88 files had drifted from it.
Scope is deliberately what this change can be verified against:
c/*.c c/*.h c/tests/*.c c/tools/*.c -- every one of these compiles here.
NOT reformatted, and the CI check added next skips them for the same
reason: *.mm has no automated coverage at all (the macOS CI 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. They can follow once there is
a gate that would catch it.
One block is fenced off with `clang-format off`: the CKR config-validation
table. 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 block one level deeper on EVERY run -- it never reaches a fixed point,
and the format check would fail on a tree that had just been formatted.
It stays the two-column table it was written as. Formatting is otherwise
idempotent: running clang-format over the whole tree again is a no-op.
Verified equivalent, not just "clang-format only touches whitespace":
the preprocessed token stream of all four engines was compared before and
after, and 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.
On top of that: all four engines build with zero warnings, make test-c
passes, test_tok_o200k is 40/40 encode + decode, and colibri.c, kimi_k3.c
and backend_vulkan.c still compile under -DCOLI_VULKAN.
Reformatting also removed every -Wmisleading-indentation warning (18 of
them, all from the compact `if (a) b; if (c) d;` style on one line), which
is what makes enabling that warning free in the next commit.
Add .git-blame-ignore-revs so `git blame` skips this commit. Configure it
locally with:
git config blame.ignoreRevsFile .git-blame-ignore-revs
CFLAGS carried -Wno-unused-parameter, -Wno-misleading-indentation and -Wno-unused-function since the beginning. Two of the three can now go: -Wmisleading-indentation: reformatting removed all 18, so enabling it is free. It is worth having on -- it is the warning that catches `if (c) a; b;` where b was meant to be guarded. -Wunused-parameter: one hit, colibri.c dense_mlp's `D`. Every operand's shape comes from the QT descriptors, so the parameter is dropped rather than voided. -Wunused-function stays off, and the remaining $(WARN_OFF) says 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. Real dead code is still findable, it is just the intersection over all four builds rather than one compiler flag (that is how the four dead functions in the previous commit were found). Python: ruff, configured in pyproject.toml. Selects pyflakes plus ruff's default pycodestyle subset, and switches off 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). What was left was 55 real findings, all fixed here: 17 unused imports, 31 f-strings with no placeholder, 4 unused locals and 3 bare `except`. The bare excepts become `except Exception`, matching the `except (ValueError, IndexError)` already used in the same function, and no longer swallow Ctrl-C. New targets: `make -C c lint` (lint-c + lint-py), runnable without a model, a GPU or a build. lint-c warns if clang-format's major version is not the one CI pins -- its output is not stable across majors, so an unpinned check would fail PRs over the contributor's distro rather than their code. CI gets a `lint` job doing both; it is the cheapest job in the file, so it reports first. make test-c passes; the Python suite is 288 tests, OK (18 skipped).
clang-tidy's bugprone-suspicious-realloc-usage found three sites doing `p = realloc(p, n)` with no NULL check, where the very next statement dereferenced the result. On allocation failure realloc returns NULL, the array pointer 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. json.h j_parse object growth (keys + kids) and array growth (kids) st.h the format-stamp table (fmt_name + fmt_val) Two of the three are in the JSON parser, which is what reads config.json -- and st.h's own comments call that an untrusted container. All three now take the result in a temporary, check it, and only then publish it. J_PUT already checked, but wrote through the same pointer it was growing; it now uses a temporary too, which is the same shape as the three above and lets the check stay enabled instead of needing a suppression (NOLINT in a macro definition does not apply at the expansion sites anyway). .clang-tidy: bugprone-* minus ten checks, each disabled with the reason recorded in the file. Those ten produce 434 findings across the four engines and not one is a defect -- they are narrowing conversions in index arithmetic that is explicitly widened where width matters, `if (!strcmp())`, `if (!(p = malloc(n)))`, _GNU_SOURCE, and the (S, D, I, O) dimension quadruple. The config is tuned so the tree passes with ZERO findings, because a check that starts with hundreds of pre-existing hits is a check nobody reads. clang-analyzer-* is deliberately not on yet. It reports path-sensitive findings that each need tracing by hand to separate a real defect from an invariant it cannot see; that is worth doing as its own change rather than smuggled in behind a lint config. New `make -C c lint-tidy` plus a CI job, kept separate from `lint` because it is a full compile plus analysis per engine -- minutes rather than seconds, and the fast formatting feedback should not queue behind it. All four engines build warning-free, make test-c passes, lint-c and lint-py are clean, and lint-tidy reports ok on all four engines.
rt_tracing was removed in a68d459 as dead code. It is not: colibri.c's device-side router calls it under `#ifdef COLI_CUDA`, so a default CPU build sees no caller while the CUDA build stops compiling. The Windows CUDA job caught it; nothing else did. Two things let that through, and both are fixed here rather than just the symptom: The intersection of "unused in all four engines" only covers the DEFAULT configuration. A reference from inside any `#ifdef COLI_*` arm is invisible to it. `make -C c check-configs` now parses all four engines under all eight combinations of the optional backends (CUDA, Vulkan, Metal, ANS). It is -fsyntax-only, so it needs no backend SDK and takes about a second -- as opposed to the five-minute Windows CUDA build that was the only job able to notice. implicit-function-declaration is a WARNING on gcc <= 13 and an ERROR on gcc >= 14. Locally (gcc 13) the missing declaration printed a warning among the build output and the engine linked anyway; on CI's newer gcc it was a hard error. check-configs forces -Werror=implicit-function-declaration and -Werror=implicit-int so the strict behaviour does not depend on how old the contributor's compiler is. Verified the gate actually catches this: deleting rt_tracing again fails check-configs on the three COLI_CUDA configurations, and restoring it passes. rt_tracing keeps a comment saying why it looks unused, so the next person reading a "dead code" report does not delete it again. Also added to the CI lint job. Everything else still passes: four engines warning-free, check-configs 4x8 ok, lint-c, lint-py, lint-tidy and test-c all clean.
c/Makefile appears in most open PRs, and any two that each append a target name to the single shared .PHONY conflict on that line by construction. That is exactly what happens between this branch, the env-registry branch, the sanitizer branch and the DeepSeek V4 work (JustVugg#772/JustVugg#773) -- four branches, one line, four conflicts that are pure bookkeeping. The TEST_RULES block above already documents this failure mode for TEST_BINS and fixes it by deriving the list rather than hand-maintaining a line. .PHONY had the same problem and never got the same treatment. make accumulates multiple .PHONY declarations, so adding a target now means adding a line instead of editing a line every other branch is also editing.
ZacharyZcR
marked this pull request as ready for review
August 3, 2026 21:11
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.
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-formatpass, 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,pbecomes 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.hj_parse, object growth (keys+kids)json.hj_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, whichst.h's own comments call an untrusted container.A
snprintftruncation that silently retargets a file write (cf72f78).route_trace.hrt_savebuilds"<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.devdoes not currently build warning-freemake kimi_k3ondevemits 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 —makeoutput just scrolls past, and only a failed build stops CI. Fixed incf72f78; thesnprintfone 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 (
attentionappears 44 times in comments and 0 times as a call).-Wunused-functionis 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_initandst_read_raw_caplook 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_tracingis 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-declarationis 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-configsnow 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 deletingrt_tracingagain: 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 incolibri.cvk_resolve_spvandkimi_k3.ck3_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_letterswaso2_letterswith Han masked out of S1/S2: the same 31 lines of regex-backtracking replay, twice. Likewisepretok_chunk_kimiwaspretok_chunk_o200kplus 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_budget2duplicated the wholeVK_EXT_memory_budgetquery forG2.Refused, because sharing them would produce worse code than the duplication:
arena_suballoc_d2differs in device, memtype, arena head and memory-priority support — sharing needs a six-argument function.attention_absorb_projectis absorb followed by an output projection pass, not a copy of absorb.rans_kernel_scalar_bfis 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-formathas been in the repo from the start and nothing ran it, so all 88 files had drifted.Not reformatted, on purpose:
*.mmhas no automated coverage at all (the macOS job builds the default CPU target, which does not compilebackend_metal.mm) and*.cuneeds 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.CKRexpands to a bracedifwith no trailing semicolon, so clang-format reads eachCKRas 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.hrewrites got their own cross-check against the previous code: 13.2Mo2_letters/km_letterscalls and 900k pretok splits across all three tokenizer families, zero divergence..git-blame-ignore-revsis included; enable it withgit 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-indentationcosts nothing now — reformatting removed all 18 — and it is the warning that catchesif (c) a; b;wherebwas meant to be guarded.-Wunused-parameterhad one hit.-Wunused-functionstays 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.ofor 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 (lis the layer index,I/Othe 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-tidyis tuned to zero findings on the current tree. The fullbugprone-*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-usageis 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) andmake -C c lint-tidy(separate, because it is a full compile plus analysis per engine). Two CI jobs to match;lintis the cheapest job in the file, so it reports first.lint-cwarns 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-cpasses,test_tok_o200kis 40/40 encode + decode, the Python suite is 288 tests OK (18 skipped), andlint-c/lint-py/lint-tidyare 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.
*.mmand*.cuare untouched.One pre-existing warning is left alone:
make colibri CUDA_DLL=1reportsg_cuda_raw_expertsdefined but not used, because its only reader sits under#ifdef COLI_ANS. That is ondevtoday and is not something this PR introduced, so it is mentioned rather than changed.On reviewing this
003a0b9is 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.