Skip to content

build: carry -Werror=switch in build.sh and the sanitizer/valgrind legs - #786

Merged
InauguralPhysicist merged 1 commit into
InauguralSystems:mainfrom
Nitjsefnie-OSC:fix/werror-switch-flag-alignment
Aug 1, 2026
Merged

build: carry -Werror=switch in build.sh and the sanitizer/valgrind legs#786
InauguralPhysicist merged 1 commit into
InauguralSystems:mainfrom
Nitjsefnie-OSC:fix/werror-switch-flag-alignment

Conversation

@Nitjsefnie

Copy link
Copy Markdown
Contributor

What does this PR do?

The follow-up you okayed on #779: ./build.sh and the asan|asan-http|tsan|valgrind legs now carry -Werror=switch, so the gate bites on the builds a contributor actually runs locally rather than only in CI via make http|gfx|zlib|lsp|full.

The line numbers in #779's note had drifted, and there were more lists than it said. Makefile:184/206/219/233 are the target-name lines; the recipes are one line lower in two cases. And build.sh has three flag lists, not one. Actual edits:

file:line leg change
Makefile:185 asan after -fsanitize=address,undefined
Makefile:206 asan-http after -fsanitize=address,undefined
Makefile:220 tsan after -fsanitize=thread
Makefile:234 valgrind before -g -O1
build.sh:45 · :55 · :67 lsp · full · minimal after -Werror=implicit-function-declaration

Flag ordering follows Makefile:9's CFLAGS. No shared variable was factored out — the only shared list is $(CFLAGS) and these seven deliberately don't use it, so leaving them alone keeps the diff to the flag itself. No other warning flag touched.

Checklist

  • make test passes locally — RESULTS: 3275/3275 passed, 0 failed
  • New builtins have signature comments and docs in docs/BUILTINS.md — n/a, build-flag change
  • New library functions follow conventions in docs/STDLIB.md — n/a
  • New examples have a comment header explaining what they demonstrate — n/a
  • CHANGELOG.md updated (if user-facing change) — n/a, no user-visible behaviour change; the compiler gate is a contributor-facing build detail

The evidence that matters is that every leg still compiles with the gate armed, since that is the whole risk of the change:

  • CC=gcc ./build.shEigenScript 0.34.0 built. Binary: 764K; CC=clang ./build.sh788K
  • make asan(asan+ubsan) built · make asan-http(asan+ubsan, http+model) built · make tsan(tsan) built
  • Regression check on the legs that already carried it: make http880K, make gfx808K, make zlib772K, make lsp924K, make full880K
  • Zero switch diagnostics anywhere. Nothing was non-exhaustive on a path CI had not been checking, which is the reassuring outcome but not one I would have asserted without building each leg.

One leg I could not verify locally, stated plainly: make valgrind does not build on this box — src/eigenscript.c:29:10: fatal error: valgrind/memcheck.h: No such file or directory, because valgrind is not installed and I did not install system packages. That is a missing toolchain, not a switch failure: compilation dies at preprocessing before any switch is seen. Your ci.yml:455-457 installs valgrind, so that leg gets exercised there. The -DEIGS_VALGRIND code behind it is memcheck macro calls rather than switch logic, so I would be surprised if it bites — but I have not proven that and am not claiming it.

Follow-ups / Known Limitations

Six more hand-written flag lists still omit -Werror=switch: Makefile:148 (jit-smoke), :251 (poison), :301 (coverage), :322/:339 (the fuzz harnesses), :364 (freestanding). They are outside what you approved, so I left them — happy to send them as a second pass if you want the whole perimeter.

Unrelated pre-existing warnings noticed while building, not introduced here and not failing anything: lint.c:813/:832 -Wcomment (/* within a comment), lint.c:899 -Wformat-truncation, and under clang step.c:491 -Wunused-but-set-variable on int fed = 0;.

Generated by Claude Opus 5 (brief, review), Kimi K3 (implementation)

InauguralSystems#779 made -Werror=switch a hard CI error via make http|gfx|zlib|lsp|full
(the $(CFLAGS) legs), but the legs contributors run locally use
hand-written flag lists that omit it, so a non-exhaustive ASTType switch
only warned there:

- build.sh (minimal, full, lsp compile lines)
- make asan, make asan-http, make tsan, make valgrind

Add -Werror=switch to those lists so the gate bites locally too. No
other warning flag changed; no -Werror generally. All legs still build
clean (gcc + clang), and the suite passes 3275/3275 on the armed
minimal build, so no switch on these paths is non-exhaustive.

Co-Authored-By: Kimi K3 <noreply@kimi.com>

@InauguralPhysicist InauguralPhysicist left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — approved

This is the follow-up agreed in #779, and it does exactly what it says: -Werror=switch added to the seven flag lists a contributor actually hits locally (build.sh ×3, asan, asan-http, tsan, valgrind) and nothing else. I diffed each hunk against the Makefile/build.sh on main — flag placement follows CFLAGS ordering, no other flag touched, no factoring detour.

Points in your favor:

  • Correcting my line numbers. You're right that #779's note pointed at target-name lines and undercounted build.sh's flag lists — three, not one. Good catch rather than silently following a wrong map.
  • Building every leg both compilers rather than asserting from the diff that nothing could break. "Zero switch diagnostics" is the load-bearing claim of this PR and you proved it per-leg instead of inferring it.
  • The valgrind caveat, stated plainly. Correct call not to install system packages. CI settles it: the valgrind (memcheck) leg ran on this PR and passed, so the one gap in your local sweep is now closed with evidence.

On the follow-ups:

  • The six remaining lists (jit-smoke, poison, coverage, fuzz ×2, freestanding) — yes, send the second pass. Same shape as this one is fine. The fuzz harnesses in particular compile subsets of $(SOURCES), so they're the most likely place a non-exhaustive switch could hide unchecked.
  • The pre-existing warnings you noted (lint.c -Wcomment/-Wformat-truncation, step.c unused-but-set under clang) — worth an issue so they don't get lost; they're real even if harmless today.

CHANGELOG n/a is the right call — contributor-facing build detail, matches this repo's convention.

CI: all 16 checks green. Merging.

@InauguralPhysicist
InauguralPhysicist merged commit 07d98e6 into InauguralSystems:main Aug 1, 2026
16 checks passed
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.

2 participants