build: no warnings in first-party C++ (#1408)

[aaylward]

Closes #1408.

build --per_file_copt=^domains/.*@-Wall,-Wextra,-Werror
build --features=external_include_paths

Scoped to domains/ like the thread-safety -Werror above it, for the reason that comment already gives: external code is not ours to fix.

The anchor is the load-bearing character

--per_file_copt matches the exec path, and generated sources live under bazel-out/*/bin/domains/…, so an unanchored domains/.* also gates code nobody here writes — 21 of the 199 compiled sources: smithy codegen, and cards.pb.cc. cc_proto_library has no copts attribute, so proto codegen would have had no local way out at all.

Excluding them afterwards does not work: the filter matches the owner label as well as the path, so -bazel-out.* leaves the label match intact and changes nothing. Anchoring is the fix, measured per compile action:

	compile actions	carrying -Werror
hand-written cards/	22	22
generated .pb.cc	18	0
generated smithy	4	0

The pre-existing thread-safety line carried the identical over-match, so it is anchored too — a deliberate change to a neighbouring flag, called out here rather than slipped in. Generated code loses -Werror=thread-safety-analysis, which is a no-op: codegen carries no ABSL_GUARDED_BY annotations.

-Werror is per translation unit, so dependency headers needed exempting

Scoping by source path does not scope by code ownership. This repo emitted every dependency include as -I/-iquote, never -isystem, so a warning inside a third-party header was fatal in our compile — demonstrated with a probe flag that produced a -Wsign-compare error inside gtest.h. --features=external_include_paths puts those headers behind -isystem.

Verified three ways, because a change that suppresses warnings is exactly the kind that can quietly disarm the thing this PR exists to enforce:

• the gtest.h probe now builds clean;
• aquery shows 28 -isystem flags on that target, where the count before was 0;
• an unused static function added to shortener.cc still fails with error: unused function [-Werror,-Wunused-function].

Across the tree, warnings fall from 1,664 to 248. The residue is external source files (186 postgres, 62 protobuf) — -isystem exempts third-party headers, not third-party .c/.cc, which is the right boundary.

-Wall does nothing, and never did

toolchains_llvm passes -Wall to every compile unconditionally (cc_toolchain_config.bzl), which aquery confirms — it appears twice on first-party sources, once on external. Stronger still: building the first-party tree with -Wno-all produces zero diagnostics, so there are no -Wall-class findings here at all. The 20 findings below all came from clang's default-on set, which -Werror promotes independently.

-Wall stays as portability insurance if the toolchain moves; it is not doing work today and I would rather say so than imply it swept something up. -Wextra is a different story — the toolchain does not pass it, and it is where most of this diff comes from.

The 20 from -Werror alone, all in one_d4_worker

12 × -Wdeprecated-declarations — Abseil deprecated MutexLock's pointer constructor. Six sites in lease_core.cc. Semantically identical, not merely compiling: the deprecated constructor delegates to the reference one (MutexLock(Mutex* mu) : MutexLock(*mu) {}), so there is no second code path. Verified thread-safety analysis still binds through the new form with a negative control — deleting a lock line still produces the expected requires holding mutex errors.

2 × -Wunused-function — Or and PinTypeOf in pg_game_sink.cc, dead copies of live helpers in occurrence_writer.cc. Both were internal-linkage on both sides, so there was never a cross-TU relationship to break.

2 × -Wunused-result — an ignored [[nodiscard]] on absl::SimpleAtoi, in pg_queue.cc and reanalysis_queue.cc.

The parse fixes are defensive, not live bugs

SimpleAtoi leaves its out param unspecified on failure, and abseil's implementation writes meaningful wrong values into it — measured against this exact version:

input	before	after
"12x"	12	0
"3.9"	3	0
"99999999999999"	2147483647	0

The int parsed = 0 initialiser looks like protection and isn't, and the INT_MAX case fed report.games_processed += … — signed-overflow UB and a negative counter.

But it is unreachable. Every column feeding these is Postgres INT (V015__dispatch_columns.sql, V017__reanalysis_requests.sql), whose text output is always well-formed int32, and NULL already mapped to 0 on both paths. So no caller relied on the old behaviour and none could have. An earlier draft of this description called it a live bug; it is a real defect in the function, on input the schema cannot currently produce.

The same reasoning covers MonthAlreadyIndexed, which parsed games_count with std::stoi. That one throws, and the worker contains no try/catch — a malformed row would have taken the process down rather than answered wrong. games_count is INT NOT NULL, so equally unreachable, but the function already returns absl::StatusOr, so it costs nothing to answer with a status. That was the last throwing parse in first-party C++.

Consequence worth stating: no test covers these branches — reverting them leaves CI green. ToInt is file-local with no seam, and extracting a three-line helper purely to exercise a branch production cannot reach seemed the worse trade. Flagging it rather than leaving it implicit.

-Wextra: 65 findings, three checks, 16 files

The toolchain does not pass -Wextra, so these are genuinely new. They fall into three checks and none of them is a live bug — this is the cost of the flag, stated as a cost.

55 × -Wunused-parameter. Overrides and default no-op hooks that ignore part of an interface they must still accept: test fakes in run_ceiling_test, poller_test, hub_e2e_test, the context argument smithy hands every operation, and the RunObserver/Detector default hooks whose whole point is that a subclass overrides only what it cares about. [[maybe_unused]] rather than dropping the name, because the doc comments on those two interfaces refer to the parameters by name.

This is the check with an ongoing tax: every future no-op override needs the annotation. It is the standard reason people adopt -Wextra with -Wno-unused-parameter, and it is a one-line change if it grates. I did not make it, because #1408 says no warnings, not no warnings except the tedious ones.

9 × -Wmissing-designated-field-initializers, fixed in 4 places. Clang flags a designated initializer that omits a field — but only when that field has no default member initializer. So every one of these is fixed at the struct, which covers all present and future callers and states the default the code was already relying on:

field	was	now
OtelConfig::histogram_bounds	std::map<…> histogram_bounds;	= {}
ChainOptions::allow_request	std::function<…> allow_request;	= nullptr
AppContext::selected	SDL_FRect selected;	= {}

ChainOptions::allow_request is the clearest case: its own doc comment says "Leave allow_request unset for services without a rate limiter", so the warning was firing on documented intent. The NSDMI makes the documented default the declared one.

The fourth is smithy's BeastServerTransport::RejectedRequest, whose three std::string fields have no NSDMIs. Not ours to edit, so the one call site names them explicitly — filed upstream as muchq/smithy-cpp#193, which would let that revert to the terse form.

1 × -Wmissing-field-initializers. tracy::constants::UNSET was RGB_Double{-1.0}, leaving g and b to zero-init. Spelled {-1.0, 0.0, 0.0} — the same value, because UNSET is compared with operator== against real colours and changing the sentinel would change which colours match it.

How this landed in two commits, and why the first one was wrong

I pushed -Wextra claiming zero first-party findings. That was false, and CI caught it within minutes: tracy.h:18 failed the tsan job.

The claim came from a grep over the build log anchored as ^domains/. Two things defeat it. Clang reports a header reached through a relative include as ./domains/…, and bazel writes ANSI colour codes into a redirected log, so the line actually begins \x1b[1mdomains/…. The pattern matched neither, returned zero, and I read the zero as a result instead of as a pattern that had never matched anything. The build summary on the same screen said 30 fail to build; I did not reconcile the two.

The rebuilt measurement strips ANSI, accepts the ./ prefix, and iterates to a fixed point — a compile aborts after roughly twenty errors, so one pass under-reports. It is mutation-checked: removing a single [[maybe_unused]] from index_run.h fails the build with unused parameter 'game' [-Werror,-Wunused-parameter]. And every subsequent zero-warning claim here was checked against a known-bad log first, so a zero means zero rather than a pattern that never matched.

Review panel

Three lenses, run against the -Werror half of this change. The anchoring finding above is the one that changed the diff; the header exposure and the std::stoi site were the other two survivors, both folded in here. Also cleared: no first-party C++ exists outside //domains/... (one vendored header, no compile action of its own); no exec-config C++ compiles, so --host_per_file_copt is not needed; sanitizer copts land before the per-file ones and compose fine; iOS carries no C++; there are no select()ed C++ sources or OS #ifdefs; and the sweep is complete — zero MutexLock(& remain under domains/, and every parse of a database column now checks its result.

The panel did not cover the -Wextra commit, which came later — saying so rather than letting the section imply otherwise. That commit's evidence is the mutation check, the clean full build, and the test suite, all below.

CLAUDE.md gains the escape hatch under "Things that bite": add a -Wno-error= line below this one, since later flags win. The obvious alternative does not reliably work — copts on the target can be re-enabled by this flag's -Wall landing afterwards, and no C++ target in this repo has ever used copts.

Verified

On the rebased base (main at f18cb09, which brings #1439's C++ under these flags for the first time): bazel test //domains/... //bazel/... → 218 tests pass, and a full bazel build --keep_going //domains/... over 626 targets reports zero diagnostics from first-party files. #1439's code needed no changes.

One first-party source is not covered locally: hello_raylib.cc, because @raylib//:raylib_cmake needs libxrandr headers this sandbox lacks, so nothing downstream of it compiles here. It is 17 lines with no parameters and no aggregates, and CI builds it.

CI runs the full //... here — .bazelrc is in FULL_BUILD_PATHSPECS, so diff-build takes the full-build branch rather than an impact subset — and the sanitizer jobs cover -c dbg, which my local -c opt runs do not. That is the configuration that caught tracy.h, and all three sanitizers are green on the rebased head.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ST7XLGWstRE4CfUF84ijKD

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	1d4-web	8404df4	Commit Preview URL Branch Preview URL	Aug 23 2026, 12:54 PM

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
❌ Deployment failed View logs	iili	50c5222	Aug 23 2026, 03:11 AM

[aaylward]

Workers Builds: iili is red here and it is not this PR. The diff is .bazelrc plus four C++ files in one_d4_worker; it touches nothing under domains/iili/. test-iili-web — the Actions job that builds that app from its real path — passes on the same commit.

It is the leftover from #1435: the iili Workers project's Root directory still points at /domains/r3dr/apps/iili_web, which stopped existing on main when the rename merged. So the check now fails on every PR against main regardless of content, not just on the rename PR where it started.

Production is unaffected — iili.uk is serving current code (its live bundle calls /iili/v1/shorten and mints i.iili.uk/r/), so the Worker was deployed some other way. What is lost is the automatic build: pushes to main will not redeploy that Worker until the setting is corrected to /domains/iili/apps/iili_web.

---

Generated by Claude Code

[aaylward]

Correcting my comment above: the Workers setting is not stale — it reads /domains/iili/apps/iili_web and has for a while. The failing build's own Build settings panel shows it ran with /domains/r3dr/apps/iili_web, so Cloudflare captured the configuration when the build was created and this build predates the edit. Same failure, different cause than I wrote: a stale snapshot, not a stale setting.

The rest stands — the red check is unrelated to this diff, and test-iili-web passes on the same commit. A build from a new push picks up the corrected settings; a retry of build #33770d28 may reuse its snapshot.

---

Generated by Claude Code

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	iili	8404df4	Commit Preview URL Branch Preview URL	Aug 23 2026, 12:55 PM