bcox/tymbal
1
0
Fork 0
Code Issues 97 Pull requests Projects Releases Packages Wiki Activity Actions

patches/audition.cc: stale bank count in the allocation comment #63

New issue
Closed
opened 2026-09-05 12:31:37 +00:00 by bcox · 2 comments
bcox commented 2026-09-05 12:31:37 +00:00
Owner
Copy link

patches/audition.cc:200 reads:

// Not free: 28 allocations of a 200-voice pool instead of one. This is an

The bank is 30 patches as of modal-cymbals (it was 29 before, and 28 before
that), so the number has been stale through two additions. Bank.HasThirtyPatches
in patches/patches_test.cc is the live count.

Trivial, comment-only. Worth doing with a sweep for the same shape elsewhere:
this class of stale count has now been found three times in this repo --
CLAUDE.md's "29 patch names" (fixed on modal-cymbals), the
EveryBlobsMacroKnobsAreItsOwn header's blob counts (fixed on the same
branch), and this one. A count that has to be updated by hand on every patch
addition is a count that will go stale; if any of these can be derived from
patches::Count() instead, that is the better fix.

Found by the whole-branch review of modal-cymbals.

`patches/audition.cc:200` reads: // Not free: 28 allocations of a 200-voice pool instead of one. This is an The bank is 30 patches as of `modal-cymbals` (it was 29 before, and 28 before that), so the number has been stale through two additions. `Bank.HasThirtyPatches` in `patches/patches_test.cc` is the live count. Trivial, comment-only. Worth doing with a sweep for the same shape elsewhere: this class of stale count has now been found three times in this repo -- `CLAUDE.md`'s "29 patch names" (fixed on `modal-cymbals`), the `EveryBlobsMacroKnobsAreItsOwn` header's blob counts (fixed on the same branch), and this one. A count that has to be updated by hand on every patch addition is a count that will go stale; if any of these can be derived from `patches::Count()` instead, that is the better fix. Found by the whole-branch review of `modal-cymbals`.
bcox commented 2026-09-05 23:17:49 +00:00
Author
Owner
Copy link

The bank is 31 as of branch fm-followups, which adds patches/chowning_horn.h -- so patches/audition.cc:200's "28 allocations" is now three additions stale rather than two.

That branch also introduces a second instance of the same shape: patches/patches_test.cc:264 reads "a 200-voice pool per patch to serialize 30 patches", which the branch itself made false. The live count moved correctly (Bank.HasThirtyOnePatches); only the prose counts drift.

This is the fourth and fifth instance of this shape in the repo, which strengthens the issue's own recommendation: derive these from patches::Count() rather than fixing the numbers again.

The bank is **31** as of branch `fm-followups`, which adds `patches/chowning_horn.h` -- so `patches/audition.cc:200`'s "28 allocations" is now three additions stale rather than two. That branch also introduces a second instance of the same shape: `patches/patches_test.cc:264` reads "a 200-voice pool per patch to serialize 30 patches", which the branch itself made false. The live count moved correctly (`Bank.HasThirtyOnePatches`); only the prose counts drift. This is the fourth and fifth instance of this shape in the repo, which strengthens the issue's own recommendation: derive these from `patches::Count()` rather than fixing the numbers again.
bcox commented 2026-09-09 23:56:25 +00:00
Author
Owner
Copy link

Fixed on worktree-audition-flags, together with #88, #69 and #81 — and this issue's own figures make the case better than it argued.

It reports the comment as stale at 28 against a bank of 30, and names Bank.HasThirtyPatches as the live count. The bank is 40 today and the assertion is Bank.HasFortyPatches (patches/patches_test.cc:3309, renamed at 109447b). So between the report and the fix the number went stale twice more — 28 → 29 → 30 → 40 — including in the issue written to complain about it going stale.

So it is dropped rather than corrected (bcox's call: "It's going to immediately go stale and we'll have to keep updating it"). The comment now reads:

// Not free: one 200-voice pool per patch rather than one for the whole
// run. This is an offline tool that renders the bank and exits, so that is
// not a cost worth a reused instrument's ambiguity. //patches:install
// still reuses one, and its own comment says what makes that safe.

The "about 110 seconds of audio" in the same sentence went too — it is the same bank-sized quantity wearing different units.

The sweep this issue asked for. No other count of this shape survives in patches/: everything count-shaped there is a format minor, a Q-format width, or a frozen measurement ("Twelve of the twenty-eight printed peaks moved when both leaks were closed" is a record of what a change did, correctly frozen at the figure of the day, and is left alone). Bank.HasFortyPatches stays as it is — a count inside an assertion cannot go stale silently, because the build tells you.

The fourth instance of the class was CLAUDE.md's test-target count, which is #81, fixed on the same branch by removing the number and saying why it is not there.

Fixed on `worktree-audition-flags`, together with #88, #69 and #81 — and this issue's own figures make the case better than it argued. It reports the comment as stale at 28 against a bank of 30, and names `Bank.HasThirtyPatches` as the live count. **The bank is 40 today** and the assertion is `Bank.HasFortyPatches` (`patches/patches_test.cc:3309`, renamed at `109447b`). So between the report and the fix the number went stale twice more — 28 → 29 → 30 → 40 — including in the issue written to complain about it going stale. So it is dropped rather than corrected (bcox's call: "It's going to immediately go stale and we'll have to keep updating it"). The comment now reads: ``` // Not free: one 200-voice pool per patch rather than one for the whole // run. This is an offline tool that renders the bank and exits, so that is // not a cost worth a reused instrument's ambiguity. //patches:install // still reuses one, and its own comment says what makes that safe. ``` The "about 110 seconds of audio" in the same sentence went too — it is the same bank-sized quantity wearing different units. **The sweep this issue asked for.** No other count of this shape survives in `patches/`: everything count-shaped there is a format minor, a Q-format width, or a frozen measurement (`"Twelve of the twenty-eight printed peaks moved when both leaks were closed"` is a record of what a change did, correctly frozen at the figure of the day, and is left alone). `Bank.HasFortyPatches` stays as it is — a count inside an assertion cannot go stale silently, because the build tells you. The fourth instance of the class was `CLAUDE.md`'s test-target count, which is #81, fixed on the same branch by removing the number and saying why it is not there.
bcox closed this issue 2026-09-10 00:09:21 +00:00
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
eq-layers
leslie
coverage
ci
No results found.
Labels
Clear labels   
designed-for

The codebase already has the seam; this uses it

  
excluded-by-design

Decided against; recorded so the reasoning is findable

  
infrastructure

Build, CI, tooling

  
performance

   
proposal

Discussed and plausible; nobody has asked for it yet

No labels
designed-for
excluded-by-design
infrastructure
performance
proposal
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
bcox/tymbal#63
No description provided.