Forward _useRingBuffers through Esai/Essi to Audio - #3
Open
elijahr wants to merge 5 commits into
Open
Conversation
The Audio base class carries a _useRingBuffers flag (default true) that controls whether the blocking ring-buffer callbacks are installed at construction. Esai and Essi (the concrete audio peripherals) did not forward this flag, so an embedder could not opt out without rewriting callbacks after construction. This change: - Adds _useRingBuffers (default true) to the Esai and Essi constructors - Adds a forwarding ctor to Esxi (pure interface) that takes the flag and forwards it to the Audio base - Preserves all existing call sites (default true keeps the old blocking path) Verified: - cmake --build build --target dsp56kEmu: clean (only pre-existing asmjit warnings) - cmake --build build --target dsp56kTestRunner: clean - dsp56kTestRunner: 269/269 assembler tests PASS, all JIT/Interpreter/ JIT-Optimizer tests PASS, exit 0 - reference/dsp56300 untouched - No edits to plan or AGENTS.md Committed by orchestrator on behalf of up4-repair per stopgap discipline.
The repository had no AGENTS.md. This one records the build and test commands, the architecture, and the conventions that are not derivable from the source, and it carries the comment rule the operator set today. Because this repository is a fork, the file states the authorship boundary as the command that computes it rather than as a literal, and says that a comment inherited from upstream is never swept. The boundary names upstream/dsp56300, which is where upstream's HEAD points. An earlier draft named upstream/main. That ref yields more than twice the paths, and most of the extra ones are upstream's own work, so a sweep run against it would have edited other people's comments. The pinned commit was correct for the ref it named; the ref was the defect.
A comment that states a count of things in the tree goes stale on the next change and nothing catches it. The same rule the other repositories in this project now carry. This is a fork, so the boundary is the merge base against upstream/dsp56300 and it was applied per line, not per file. Blame attributes twenty of the comment lines in the inherited files to us; the other three hundred and twenty-two are upstream's and were never candidates. A boundary drawn per file would have put all of them in scope. Two comments were already false. peripherals56311.h cited an assert at dsp.cpp line 88, which holds a constructor initialiser; the assert is two lines further down. The trigger-overload test called checkTrigger two free functions where there are three. Kept, because a mechanism reads each: the request-source assertions, the CTest version floor the same step compares against, and the compile-time checks on the 56311 peripheral set. Their rationale comments are kept with them. Not changed, and reported instead: the peripheral-type test asserts that a hand-written array in the test file holds five entries, which says nothing about the enumeration it names. The enumeration carries no count sentinel, so the guard cannot be repaired without changing the header first. Its comments are left in place because they are the only description of what the guard was meant to do. No behaviour change. Every added and removed line is a comment, proven by filtering the diff. Configure, build and the test suite all pass.
The earlier sweep looked for digits and for the word "count". It did not look for numbers spelled out, so files that read as swept still carried them. A survivor in a swept file is worse than one in an untouched file: a reader who knows the sweep ran takes it for a number somebody examined and kept. Most word-form numbers here describe the part rather than this tree. Eight slots, two-word jumps and the second bus are hardware facts and they stay. What goes is the arithmetic about our own files and tests. Where removing a number left a sentence without a subject, the sentence is rewritten rather than left dangling. AGENTS.md gains the build and test commands, narrow first and full second, with the traps that apply to this repository. It also carries the comment rule, and the two classes the first version of that rule missed: an enumeration is a count with the number spelled out, and a comment that names a path must name one that exists. No behaviour change. Comments were stripped from the committed copy and from the working copy and the results compared, with string literals masked so a trailing comment cannot hide a code edit. The stripper's failure path was exercised rather than assumed. The suite passes as before.
|
✅ Momus review posted — verdict APPROVE, 1 finding
|
Author
|
/ai-review |
![axiomantic-momus[bot]](https://avatars.githubusercontent.com/u/230757868?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR forwards a new _useRingBuffers constructor parameter from Esai/Essi through Esxi to Audio (defaulting to true, so existing behavior is unchanged), adds an AGENTS.md, and deletes stale-prone and count-bearing comments from fork-authored files. The plumbing is correct and default-preserving, but the new behavior-selecting parameter has no test coverage, and the PR also removes the pull_request: opened trigger from .github/workflows/momus.yml without mentioning it in the PR body.
Severity tally: 1 Low.
Low
- BOT-A1 (
source/dsp56kEmu/esxi.h:12): New_useRingBuffersparameter has no test coverage, so a dropped forward would silently ignore the flag.
Noteworthy
- The constructor plumbing is minimal and default-preserving, so no existing call site changes behavior.
Verdict: APPROVE.
Commands
- Comment
/ai-reviewor mention @axiomantic-momus[bot] to request a re-review of the latest changes. - Reply to a finding with
won't fix,by design, ornot a bugto decline it. - Reply with
instead, ...to propose an alternative fix.
Cost: $0.14 - 256,990 in / 26,376 out tokens - deepseek/deepseek-v4-pro
Powered by Momus running deepseek/deepseek-v4-pro via openrouter.ai.
| class Esxi : public Audio | ||
| { | ||
| public: | ||
| explicit Esxi(bool _useRingBuffers = true) : Audio(_useRingBuffers) {} |
There was a problem hiding this comment.
BOT-A1 — Low (tests)
New _useRingBuffers parameter has no test coverage, so a dropped forward would silently ignore the flag.
The PR exposes a behavior-selecting public parameter on Esai/Essi — explicit Esxi(bool _useRingBuffers = true) : Audio(_useRingBuffers) {} (source/dsp56kEmu/esxi.h:12) — that flips Audio between ring-buffer and callback mode. A repo-wide grep for hasRingBuffers returns only the definition bool hasRingBuffers() const { return m_useRingBuffers; } in source/dsp56kEmu/audio.h:126 and zero hits in source/dsp56kEmu/test/. Because the default is true, a regression that drops the Esxi(_useRingBuffers) forward (or passes the wrong variable) would compile cleanly and silently ignore a false request — exactly the failure mode this repo's convention ('a failing test is the only durable statement about coverage') is meant to catch.
Suggested change
| explicit Esxi(bool _useRingBuffers = true) : Audio(_useRingBuffers) {} | |
| // add to a dsp56k_* test source | |
| void theRingBufferFlagIsForwarded() | |
| { | |
| Peripherals56303 periph; | |
| Essi essi(periph, 0, false); | |
| verify(!essi.hasRingBuffers()); | |
| Essi defaulted(periph, 0); | |
| verify(defaulted.hasRingBuffers()); | |
| } |
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
1 participant
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.
Ring-buffer plumbing and repository notes. Four commits, based on
dsp/stack-02.feat(esxi): forward_useRingBuffersthroughEsai/EssitoAudioAGENTS.mdThe comment deletions are confined to code this fork authored; upstream comments are left alone.
PR 3 of a 6-PR stack. Merge after
dsp/stack-02.