From dddecc57347950b93890acbad4ab8c24dfc8cca2 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 30 Jul 2026 21:03:02 -0400 Subject: [PATCH] =?UTF-8?q?note:=20close=20out=20the=20model=20=E2=80=94?= =?UTF-8?q?=20correct=20an=20inert=20mutation=20claim,=20retag=20four=20no?= =?UTF-8?q?n-discriminating=20assertions,=20assert=20Tempo's=20closure,=20?= =?UTF-8?q?fix=20three=20doc/test=20accuracy=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No behavior change; verification-record corrections and one static_assert. --- src/core/instrument/note/CLAUDE.md | 21 +++++++++++++-------- src/core/instrument/note/note_program.cpp | 6 ++++-- src/core/instrument/note/tempo.h | 2 ++ tests/test_musical_division.cpp | 5 +++++ tests/test_note_program.cpp | 20 ++++++++++++++++++-- 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/core/instrument/note/CLAUDE.md b/src/core/instrument/note/CLAUDE.md index bc66354..895e8f5 100644 --- a/src/core/instrument/note/CLAUDE.md +++ b/src/core/instrument/note/CLAUDE.md @@ -31,14 +31,16 @@ diverge: the capture-signal popup that edits it and the bake that renders it. the ladder ever gained a rung or a modifier. - **An offset stores the denomination it was entered in** — see `OffsetAmount` in `note_program.h` for why. -- **Every value type establishes its domain at construction, and nothing downstream can - fail.** `Tempo::fromBpm` rejects, alone, because an unusable BPM has no nearest usable one - to fall to. `Division`, `OffsetAmount`, and `Velocity` clamp, because an off-ladder rung, - an unrepresentable magnitude, and an out-of-range velocity each do. Each has exactly one - door (`makeDivision`, `offsetOf`, `Velocity::of`) and a private constructor behind it, so - an out-of-domain value cannot be held, only passed in. That is what lets every reader - branch without a fallback, equality compare fields raw, and `resolveNote` return finite - times for every constructible input with no failure path and no validity flag. +- **Every value type establishes its domain at construction, so every field `resolveNote` + returns is finite for every constructible program and tempo.** `Tempo::fromBpm` rejects, + alone, because an unusable BPM has no nearest usable one to fall to. `Division`, + `OffsetAmount`, and `Velocity` clamp, because an off-ladder rung, an unrepresentable + magnitude, and an out-of-range velocity each do. Each has exactly one door (`makeDivision`, + `offsetOf`, `Velocity::of`); `Division` and `OffsetAmount` block any other path with a + private value constructor, `Velocity` with a private member that only `of()` writes — + either way an out-of-domain value cannot be held, only passed in. That is what lets every + reader branch without a fallback, equality compare fields raw, and `resolveNote` return + finite times for every constructible input with no failure path and no validity flag. - **The module will not tell a caller a record is junk, because a junk record cannot exist here.** Corruption is only visible where raw bytes are: a codec sees both the bytes it read and the value construction produced, and reporting the difference is the codec's job. @@ -79,6 +81,9 @@ diverge: the capture-signal popup that edits it and the bake that renders it. Both are legal; `resolveNote` only refuses to invert the window. - **ms <-> beats round-trips are lossless to double precision, not bit-identical.** The conversion is a multiply/divide pair; compare with an epsilon. +- **`Division` and `OffsetAmount` are trivially copyable, so a `memcpy` of a wire record + bypasses every door.** Decode field-by-field through `makeDivision`/`offsetOf` (the pattern + `src/core/wire/bytes.h` already uses) instead — never `memcpy` raw bytes into either type. - **Editing the ms field of a beats-stored offset stores beats, and the ms readout will then move with the tempo.** `withMsView` keeps the stored denomination on purpose, so typing 250 into the ms field of a beats offset stores 0.5 beats at 120 BPM. That is the intended diff --git a/src/core/instrument/note/note_program.cpp b/src/core/instrument/note/note_program.cpp index 42f187a..f2b1d73 100644 --- a/src/core/instrument/note/note_program.cpp +++ b/src/core/instrument/note/note_program.cpp @@ -64,8 +64,10 @@ double offsetSeconds(OffsetAmount amount, Tempo tempo) { } OffsetAmount redenominate(OffsetAmount amount, Denomination to, Tempo tempo) { - // Route the requested target through the same door a stored denomination goes through, - // so an out-of-enum target lands where a corrupt stored one does. + // Defense-in-depth, not a discriminating guard: the branch below already treats any + // non-Beats target as Milliseconds, so an unnamed `to` resolves the same way whether or + // not it is routed through offsetOf first. Kept because a future third denomination + // would make this the one place that still pins it. const Denomination target = offsetOf(0.0, to).denomination(); if (amount.denomination() == target) return amount; return target == Denomination::Beats ? offsetFromBeats(offsetBeats(amount, tempo)) diff --git a/src/core/instrument/note/tempo.h b/src/core/instrument/note/tempo.h index a768398..c05508e 100644 --- a/src/core/instrument/note/tempo.h +++ b/src/core/instrument/note/tempo.h @@ -43,5 +43,7 @@ private: static_assert(!std::is_default_constructible_v, "Tempo must not be constructible without a validated BPM"); +static_assert(!std::is_constructible_v, + "fromBpm must be the only way to give a Tempo a value"); } // namespace reasampler::instrument::note diff --git a/tests/test_musical_division.cpp b/tests/test_musical_division.cpp index 0997af2..e620ecf 100644 --- a/tests/test_musical_division.cpp +++ b/tests/test_musical_division.cpp @@ -175,6 +175,9 @@ static void testUnnamedModifierClampsToStraight() { == divisionIndex(makeDivision(0, DivisionModifier::Straight))); CHECK(divisionLabel(makeDivision(0, junk)) == "1/4"); // and no junk reaches the readout CHECK(makeDivision(0, junk) == makeDivision(0, DivisionModifier::Straight)); + // Measured (clamp removed from clampModifier): still passes. junk(7) != Dotted's stored + // modifier either way, clamped or raw — this discriminates a degenerate operator== that + // ignores the modifier field, not the clamp itself. CHECK(makeDivision(0, junk) != makeDivision(0, DivisionModifier::Dotted)); } @@ -184,6 +187,8 @@ static void testEveryConstructibleDivisionIndexesIntoThePickerSet() { // modifier clamp. const int exponents[] = {-9000, -99, kMinQuarterExponent, 0, kMaxQuarterExponent, 120, 9000}; for (int e : exponents) { + // 260, not 256: m=256..259 wrap modulo uint8_t back to 0..3, re-covering the four + // lowest bytes rather than reaching any byte 256 alone couldn't already reach. for (int m = 0; m < 260; ++m) { const Division d = makeDivision(e, static_cast(m)); const int index = divisionIndex(d); diff --git a/tests/test_note_program.cpp b/tests/test_note_program.cpp index 9d1c3f9..4bf496a 100644 --- a/tests/test_note_program.cpp +++ b/tests/test_note_program.cpp @@ -356,14 +356,27 @@ static void testEveryDenominationBranchingFunctionAgreesWithThePin() { CHECK(corrupt == asMs); // offsetMs / offsetBeats / offsetSeconds covered above CHECK(!(corrupt != asMs)); CHECK(redenominate(corrupt, Denomination::Milliseconds, t) == corrupt); + // Measured (mutate offsetOf to a pass-through): still passes. offsetFromBeats always + // tags its result Beats, so this holds regardless of whether corrupt was pinned — it + // does not discriminate the pin. CHECK(redenominate(corrupt, Denomination::Beats, t).denomination() == Denomination::Beats); + // Reported measured (withMsView reverted to its pre-domain-closure form): still passes. + // corrupt already equals asMs by this point, and withMsView is a pure function of its + // argument, so this line cannot discriminate anything withMsView-specific — it is a + // restatement of the equality above. CHECK(withMsView(corrupt, 40.0, t) == withMsView(asMs, 40.0, t)); CHECK(withMsView(corrupt, 40.0, t).denomination() == Denomination::Milliseconds); + // Measured (mutate offsetOf to a pass-through): still passes. withBeatsView only branches + // on `== Beats`; any non-Beats value — pinned or raw corrupt — takes the same ms-based + // else branch, so this does not discriminate the pin either. CHECK(withBeatsView(corrupt, 1.0, t) == withBeatsView(asMs, 1.0, t)); CHECK(withBeatsView(corrupt, 1.0, t).denomination() == Denomination::Milliseconds); CHECK(almostEqual(withBeatsView(corrupt, 1.0, t).magnitude(), 500.0, 1e-6)); - // An unnamed TARGET denomination pins the same way an unnamed stored one does. + // Reported measured (offsetOf(0.0, to) replaced with `= to;`): still passes, for the + // same reason as above — `target == Beats` is false whether target is pinned or raw, so + // this always takes the ms branch and cannot discriminate the door (see the comment on + // that line in note_program.cpp). CHECK(redenominate(offsetFromBeats(1.0), static_cast(7), t) == offsetFromMs(500.0)); } @@ -445,7 +458,10 @@ static void testResolveNoteIsFiniteForEveryConstructibleInput() { const double magnitudes[] = {-inf, -kMaxConvertibleMagnitude, -1e300, 0.0, 1e300, kMaxConvertibleMagnitude, inf, nan}; int accepted = 0, rejected = 0; - for (double bpm : {1e-320, 1e-306, 1e-200, 1e-6, 0.5, 120.0, 1e6, 1e100, 1e308}) { + // 1e-294/1e-295 bracket the accept/reject edge (measured ~3.34e-295) so the sweep + // actually approaches it rather than jumping past it by ~95 orders of magnitude. + for (double bpm : + {1e-320, 1e-306, 1e-295, 1e-294, 1e-200, 1e-6, 0.5, 120.0, 1e6, 1e100, 1e308}) { const std::optional tempo = Tempo::fromBpm(bpm); if (!tempo) { ++rejected; continue; } ++accepted;