From a80eb76c1fb3ea7a9973750a99a93db6dc251f47 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 30 Jul 2026 20:37:12 -0400 Subject: [PATCH] note: close every value type's domain at construction, so resolveNote is finite for every constructible input Division and OffsetAmount get single normalizing doors and private constructors; fromBpm validates by running the conversions rather than their reciprocal. Readers drop their re-clamps and default labels. --- src/core/instrument/note/CLAUDE.md | 26 ++- src/core/instrument/note/CMakeLists.txt | 3 +- src/core/instrument/note/musical_division.cpp | 32 ++-- src/core/instrument/note/musical_division.h | 41 ++++- src/core/instrument/note/note_program.cpp | 65 ++++--- src/core/instrument/note/note_program.h | 45 ++++- src/core/instrument/note/tempo.cpp | 14 +- src/core/instrument/note/tempo.h | 10 +- tests/test_musical_division.cpp | 68 ++++++-- tests/test_note_program.cpp | 165 +++++++++++++++--- tests/test_tempo.cpp | 48 ++++- 11 files changed, 398 insertions(+), 119 deletions(-) diff --git a/src/core/instrument/note/CLAUDE.md b/src/core/instrument/note/CLAUDE.md index 9a00025..bc66354 100644 --- a/src/core/instrument/note/CLAUDE.md +++ b/src/core/instrument/note/CLAUDE.md @@ -31,6 +31,19 @@ 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. +- **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. + Do not add a validity flag to `NoteProgram` or `ResolvedNote` to carry that signal upward + — `windowCollapsed` describes a legal program, and is not the seed of an error channel. - **Does not carry a MIDI note number.** `NoteProgram` describes timing and velocity only; render pitch is deferred to a later additive field (Ξ-W2) rather than assumed to live here. @@ -41,9 +54,11 @@ diverge: the capture-signal popup that edits it and the bake that renders it. exponent of its length in quarter notes, -4..8), each straight, dotted (x3/2), or triplet (x2/3); the 39-entry picker order; and the `"1/8."` / `"1/4t"` label notation. Beats only — see `musical_division.h` for why it links no tempo. -- `tempo` — a validated project tempo plus every beats <-> seconds <-> ms conversion. - Construction (`Tempo::fromBpm`) is the only place a bad BPM is rejected, which is what - lets each conversion be total and every downstream resolver be failure-free. +- `tempo` — a validated project tempo plus every beats <-> seconds <-> ms conversion, and + `kMaxConvertibleMagnitude`, the beats-or-ms ceiling the whole directory caps its domains + to. `fromBpm` validates by running the extreme conversions rather than by testing the + `60/bpm` reciprocal they start from — that reciprocal stays finite well past the point the + multiply after it overflows. - `note_program` — `Velocity` (clamped 1..127), the denominated `OffsetAmount` and its unit toggle, the anchored `StartOffset` / `EndOffset`, the `NoteProgram` record, and `resolveNote`. @@ -64,3 +79,8 @@ 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. +- **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 + semantic, but it is a UI-visible surprise worth a word in the popup: `redenominate` — the + unit toggle — is the only thing that changes which denomination is stored. diff --git a/src/core/instrument/note/CMakeLists.txt b/src/core/instrument/note/CMakeLists.txt index dbffa83..f7c5540 100644 --- a/src/core/instrument/note/CMakeLists.txt +++ b/src/core/instrument/note/CMakeLists.txt @@ -6,8 +6,7 @@ reasampler_test(musical_division LINK musical_division) reasampler_pure_library(tempo SOURCES tempo.cpp) reasampler_test(tempo LINK tempo) -# note_program links exactly these two: it composes the ladder and the tempo and nothing -# else (see note_program.h). +# note_program links exactly these two: it composes the ladder and the tempo and nothing else. reasampler_pure_library(note_program SOURCES note_program.cpp LINK PUBLIC musical_division tempo) diff --git a/src/core/instrument/note/musical_division.cpp b/src/core/instrument/note/musical_division.cpp index 443d0ac..c1babad 100644 --- a/src/core/instrument/note/musical_division.cpp +++ b/src/core/instrument/note/musical_division.cpp @@ -21,28 +21,26 @@ int clampExponent(int quarterExponent) { return (std::max)(kMinQuarterExponent, (std::min)(kMaxQuarterExponent, quarterExponent)); } +// The underlying type is unsigned, so an out-of-enum byte can only be too large. +DivisionModifier clampModifier(DivisionModifier m) { + return static_cast(m) < kModifierCount ? m : DivisionModifier::Straight; +} + } // namespace bool operator==(Division a, Division b) { - // Normalize both sides through makeDivision first: a persisted off-ladder exponent must - // compare equal to its clamped form, the same as every other reader in this file. - const Division la = makeDivision(a.quarterExponent, a.modifier); - const Division lb = makeDivision(b.quarterExponent, b.modifier); - return la.quarterExponent == lb.quarterExponent && la.modifier == lb.modifier; + return a.quarterExponent() == b.quarterExponent() && a.modifier() == b.modifier(); } bool operator!=(Division a, Division b) { return !(a == b); } Division makeDivision(int quarterExponent, DivisionModifier modifier) { - Division d; - d.quarterExponent = static_cast(clampExponent(quarterExponent)); - d.modifier = modifier; - return d; + return Division(static_cast(clampExponent(quarterExponent)), + clampModifier(modifier)); } double divisionBeats(Division d) { - const Division legal = makeDivision(d.quarterExponent, d.modifier); - return std::ldexp(1.0, legal.quarterExponent) * modifierFactor(legal.modifier); + return std::ldexp(1.0, d.quarterExponent()) * modifierFactor(d.modifier()); } Division divisionAt(int index) { @@ -52,20 +50,18 @@ Division divisionAt(int index) { } int divisionIndex(Division d) { - const Division legal = makeDivision(d.quarterExponent, d.modifier); - return (legal.quarterExponent - kMinQuarterExponent) * kModifierCount - + static_cast(legal.modifier); + return (d.quarterExponent() - kMinQuarterExponent) * kModifierCount + + static_cast(d.modifier()); } std::string divisionLabel(Division d) { - const Division legal = makeDivision(d.quarterExponent, d.modifier); - const int e = legal.quarterExponent; + const int e = d.quarterExponent(); // Both branches meet at e == 2 ("1/1"): a division's written form is its length in // whole notes, which is 2^(e-2). std::string label = e <= 2 ? "1/" + std::to_string(1 << (2 - e)) : std::to_string(1 << (e - 2)) + "/1"; - if (legal.modifier == DivisionModifier::Dotted) label += '.'; - else if (legal.modifier == DivisionModifier::Triplet) label += 't'; + if (d.modifier() == DivisionModifier::Dotted) label += '.'; + else if (d.modifier() == DivisionModifier::Triplet) label += 't'; return label; } diff --git a/src/core/instrument/note/musical_division.h b/src/core/instrument/note/musical_division.h index afeaa0d..bef6fc8 100644 --- a/src/core/instrument/note/musical_division.h +++ b/src/core/instrument/note/musical_division.h @@ -6,6 +6,7 @@ #include #include +#include namespace reasampler::instrument::note { @@ -24,19 +25,43 @@ inline constexpr int kRungCount = kMaxQuarterExponent - kMinQuarterExponent + 1; inline constexpr int kModifierCount = 3; inline constexpr int kDivisionCount = kRungCount * kModifierCount; -struct Division { - std::int8_t quarterExponent = 0; // 1/4 - DivisionModifier modifier = DivisionModifier::Straight; +// The longest programmable note — the dotted top rung — so a caller composing this ladder +// with the tempo conversions can check the two domains against each other at compile time. +inline constexpr double kMaxDivisionBeats = (1 << kMaxQuarterExponent) * 1.5; + +class Division; + +// Off-ladder inputs clamp rather than reject: the only ways to reach one are a corrupt +// persisted record or a picker bug, and the nearest legal length beats a nonsense duration. +// An unnamed modifier byte has no nearest rung to fall to, so it takes the field's default. +Division makeDivision(int quarterExponent, DivisionModifier modifier); + +// In-domain by construction — `makeDivision` is the only door and it clamps BOTH fields, so +// every reader below trusts the stored pair instead of re-clamping it, and equality compares +// the two fields raw without disagreeing with any of them. +class Division { +public: + Division() = default; // 1/4 straight + + constexpr std::int8_t quarterExponent() const { return quarterExponent_; } + constexpr DivisionModifier modifier() const { return modifier_; } + +private: + Division(std::int8_t quarterExponent, DivisionModifier modifier) + : quarterExponent_(quarterExponent), modifier_(modifier) {} + friend Division makeDivision(int quarterExponent, DivisionModifier modifier); + + std::int8_t quarterExponent_ = 0; + DivisionModifier modifier_ = DivisionModifier::Straight; }; +static_assert(!std::is_constructible_v, + "makeDivision must be the only way to give a Division a rung"); + bool operator==(Division a, Division b); bool operator!=(Division a, Division b); -// Off-ladder exponents clamp rather than reject: the only ways to reach one are a corrupt -// persisted record or a picker bug, and the nearest legal length beats a nonsense duration. -Division makeDivision(int quarterExponent, DivisionModifier modifier); - -// Length in beats (quarter notes). Always > 0. +// Length in beats (quarter notes). Always > 0, and never above kMaxDivisionBeats. double divisionBeats(Division d); // Picker order: shortest rung first, straight/dotted/triplet within each rung. Index is diff --git a/src/core/instrument/note/note_program.cpp b/src/core/instrument/note/note_program.cpp index 683a356..42f187a 100644 --- a/src/core/instrument/note/note_program.cpp +++ b/src/core/instrument/note/note_program.cpp @@ -3,6 +3,7 @@ #include "core/instrument/note/note_program.h" #include +#include namespace reasampler::instrument::note { @@ -15,56 +16,69 @@ Velocity Velocity::of(int value) { bool operator==(Velocity a, Velocity b) { return a.value() == b.value(); } bool operator==(OffsetAmount a, OffsetAmount b) { - return a.magnitude == b.magnitude && a.denomination == b.denomination; + return a.magnitude() == b.magnitude() && a.denomination() == b.denomination(); } bool operator!=(OffsetAmount a, OffsetAmount b) { return !(a == b); } -OffsetAmount offsetFromMs(double ms) { return {ms, Denomination::Milliseconds}; } +OffsetAmount offsetOf(double magnitude, Denomination denomination) { + const double bounded = + std::isnan(magnitude) ? 0.0 + : (std::max)(-kMaxConvertibleMagnitude, + (std::min)(kMaxConvertibleMagnitude, magnitude)); + const bool named = denomination == Denomination::Milliseconds + || denomination == Denomination::Beats; + return OffsetAmount(bounded, named ? denomination : Denomination::Milliseconds); +} -OffsetAmount offsetFromBeats(double beats) { return {beats, Denomination::Beats}; } +OffsetAmount offsetFromMs(double ms) { return offsetOf(ms, Denomination::Milliseconds); } -// All three readers switch on Denomination with the same default (Milliseconds, the -// struct's own default value) so a corrupt persisted record reads identically everywhere — -// a popup and a bake must never disagree on an out-of-range denomination byte. +OffsetAmount offsetFromBeats(double beats) { return offsetOf(beats, Denomination::Beats); } + +// Milliseconds is pinned AFTER the switch rather than by a `default:` inside it, so the +// switch stays exhaustive over the enum and a third denomination trips switch-exhaustiveness +// diagnostics here instead of silently resolving as ms in all three. Those diagnostics are +// off at this project's warning level, so read it as a signpost — the tests are the gate. double offsetMs(OffsetAmount amount, Tempo tempo) { - switch (amount.denomination) { - case Denomination::Beats: return tempo.beatsToMs(amount.magnitude); - case Denomination::Milliseconds: - default: return amount.magnitude; + switch (amount.denomination()) { + case Denomination::Beats: return tempo.beatsToMs(amount.magnitude()); + case Denomination::Milliseconds: break; } + return amount.magnitude(); } double offsetBeats(OffsetAmount amount, Tempo tempo) { - switch (amount.denomination) { - case Denomination::Beats: return amount.magnitude; - case Denomination::Milliseconds: - default: return tempo.msToBeats(amount.magnitude); + switch (amount.denomination()) { + case Denomination::Beats: return amount.magnitude(); + case Denomination::Milliseconds: break; } + return tempo.msToBeats(amount.magnitude()); } double offsetSeconds(OffsetAmount amount, Tempo tempo) { - switch (amount.denomination) { - case Denomination::Beats: return tempo.beatsToSeconds(amount.magnitude); - case Denomination::Milliseconds: - default: return msToSeconds(amount.magnitude); + switch (amount.denomination()) { + case Denomination::Beats: return tempo.beatsToSeconds(amount.magnitude()); + case Denomination::Milliseconds: break; } + return msToSeconds(amount.magnitude()); } OffsetAmount redenominate(OffsetAmount amount, Denomination to, Tempo tempo) { - if (amount.denomination == to) return amount; - return to == Denomination::Beats ? offsetFromBeats(offsetBeats(amount, tempo)) - : offsetFromMs(offsetMs(amount, 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. + const Denomination target = offsetOf(0.0, to).denomination(); + if (amount.denomination() == target) return amount; + return target == Denomination::Beats ? offsetFromBeats(offsetBeats(amount, tempo)) + : offsetFromMs(offsetMs(amount, tempo)); } OffsetAmount withMsView(OffsetAmount amount, double ms, Tempo tempo) { - return amount.denomination == Denomination::Milliseconds - ? offsetFromMs(ms) - : offsetFromBeats(tempo.msToBeats(ms)); + return amount.denomination() == Denomination::Beats ? offsetFromBeats(tempo.msToBeats(ms)) + : offsetFromMs(ms); } OffsetAmount withBeatsView(OffsetAmount amount, double beats, Tempo tempo) { - return amount.denomination == Denomination::Beats + return amount.denomination() == Denomination::Beats ? offsetFromBeats(beats) : offsetFromMs(tempo.beatsToMs(beats)); } @@ -83,7 +97,6 @@ ResolvedNote resolveNote(const NoteProgram& program, Tempo tempo) { const double rawEndSeconds = out.noteOffSeconds + offsetSeconds(program.end.amount(), tempo); // An inverted window has no meaning to a renderer, so a far-negative end offset yields a // zero-length capture the caller can reject rather than a negative one it cannot. - // windowCollapsed distinguishes that from a genuinely zero-length program. out.windowCollapsed = rawEndSeconds < out.captureStartSeconds; out.captureEndSeconds = (std::max)(rawEndSeconds, out.captureStartSeconds); out.velocity = program.velocity.value(); diff --git a/src/core/instrument/note/note_program.h b/src/core/instrument/note/note_program.h index d0973fa..148f53b 100644 --- a/src/core/instrument/note/note_program.h +++ b/src/core/instrument/note/note_program.h @@ -13,6 +13,11 @@ namespace reasampler::instrument::note { +// The ladder and the offsets both feed the tempo conversions, so both must sit inside the +// domain fromBpm validates — checked here because this is the one file that composes them. +static_assert(kMaxDivisionBeats <= kMaxConvertibleMagnitude, + "the note-length ladder must stay inside the tempo conversions' domain"); + class Velocity { public: static constexpr int kMin = 1; // 0 is note-off in MIDI; a programmed note must sound @@ -21,7 +26,7 @@ public: Velocity() = default; static Velocity of(int value); // clamped into [kMin, kMax] - std::uint8_t value() const { return value_; } + constexpr std::uint8_t value() const { return value_; } private: std::uint8_t value_ = 100; @@ -31,20 +36,42 @@ bool operator==(Velocity a, Velocity b); enum class Denomination : std::uint8_t { Milliseconds, Beats }; +class OffsetAmount; + +// The one door. Normalizes both fields so nothing downstream has to: a magnitude past +// +/-kMaxConvertibleMagnitude clamps to it, a NaN magnitude — which names no value to clamp +// toward — becomes zero, and a denomination outside the enum becomes Milliseconds, the +// field's own default. A corrupt persisted record therefore resolves to a plausible offset +// rather than an unrepresentable one, and no two readers can disagree about which. +OffsetAmount offsetOf(double magnitude, Denomination denomination); +OffsetAmount offsetFromMs(double ms); +OffsetAmount offsetFromBeats(double beats); + // One magnitude, in the denomination it was entered in; the other view is derived on demand // and never stored. Which one was entered is itself the intent: a beats offset must follow a // tempo change and a ms offset must hold still, and only a stored denomination says which. -struct OffsetAmount { - double magnitude = 0.0; - Denomination denomination = Denomination::Milliseconds; +class OffsetAmount { +public: + OffsetAmount() = default; + + constexpr double magnitude() const { return magnitude_; } + constexpr Denomination denomination() const { return denomination_; } + +private: + OffsetAmount(double magnitude, Denomination denomination) + : magnitude_(magnitude), denomination_(denomination) {} + friend OffsetAmount offsetOf(double magnitude, Denomination denomination); + + double magnitude_ = 0.0; + Denomination denomination_ = Denomination::Milliseconds; }; +static_assert(!std::is_constructible_v, + "offsetOf must be the only way to give an OffsetAmount a value"); + bool operator==(OffsetAmount a, OffsetAmount b); bool operator!=(OffsetAmount a, OffsetAmount b); -OffsetAmount offsetFromMs(double ms); -OffsetAmount offsetFromBeats(double beats); - double offsetMs(OffsetAmount amount, Tempo tempo); double offsetBeats(OffsetAmount amount, Tempo tempo); double offsetSeconds(OffsetAmount amount, Tempo tempo); @@ -100,7 +127,7 @@ struct ResolvedNote { double noteOffSeconds = 0.0; // == the note's sounding length, note-on being 0 double captureStartSeconds = 0.0; // negative when the capture opens before the note double captureEndSeconds = 0.0; - std::uint8_t velocity = 100; // resolveNote always overwrites this; matches Velocity's own default + std::uint8_t velocity = Velocity{}.value(); // resolveNote always overwrites this // True when the programmed end offset inverted the window and resolveNote collapsed it // to zero length instead — lets a popup explain an empty window rather than just show one. bool windowCollapsed = false; @@ -108,6 +135,8 @@ struct ResolvedNote { double captureLengthSeconds() const { return captureEndSeconds - captureStartSeconds; } }; +// Total: every field of the result is finite for every constructible program and tempo, +// which is why there is no failure path here. See this directory's CLAUDE.md. ResolvedNote resolveNote(const NoteProgram& program, Tempo tempo); } // namespace reasampler::instrument::note diff --git a/src/core/instrument/note/tempo.cpp b/src/core/instrument/note/tempo.cpp index bb7e5e4..bf660bc 100644 --- a/src/core/instrument/note/tempo.cpp +++ b/src/core/instrument/note/tempo.cpp @@ -11,11 +11,15 @@ constexpr double kSecondsPerMinute = 60.0; std::optional Tempo::fromBpm(double beatsPerMinute) { if (!std::isfinite(beatsPerMinute) || beatsPerMinute <= 0.0) return std::nullopt; - // A subnormal BPM is finite and positive but overflows 60/bpm to +inf, which then turns - // any beatsToSeconds(0) into NaN downstream — reject it here so every conversion below - // stays total. - if (!std::isfinite(kSecondsPerMinute / beatsPerMinute)) return std::nullopt; - return Tempo(beatsPerMinute); + // Guard by running the conversions, not by testing the 60/bpm reciprocal they start + // from: that reciprocal stays finite for BPMs whose beatsToMs has already overflowed, + // because the conversions scale it by up to kMaxConvertibleMagnitude. Both directions + // are checked — one overflows at an absurdly slow tempo, the other at an absurdly fast + // one. Calling them here is what keeps the guard from drifting away from what they do. + const Tempo candidate(beatsPerMinute); + if (!std::isfinite(candidate.beatsToMs(kMaxConvertibleMagnitude))) return std::nullopt; + if (!std::isfinite(candidate.msToBeats(kMaxConvertibleMagnitude))) return std::nullopt; + return candidate; } double Tempo::secondsPerBeat() const { return kSecondsPerMinute / bpm_; } diff --git a/src/core/instrument/note/tempo.h b/src/core/instrument/note/tempo.h index 7c2f8a6..a768398 100644 --- a/src/core/instrument/note/tempo.h +++ b/src/core/instrument/note/tempo.h @@ -16,10 +16,16 @@ inline constexpr double kMsPerSecond = 1000.0; constexpr double msToSeconds(double ms) { return ms / kMsPerSecond; } constexpr double secondsToMs(double seconds) { return seconds * kMsPerSecond; } +// The largest magnitude, in beats or in milliseconds, the conversions below are required to +// keep finite. `fromBpm` validates against it and every caller caps its own domain to it, so +// the two halves of the totality claim meet at one number. Astronomically above anything +// musical — a billion milliseconds is eleven days — so nothing real is excluded. +inline constexpr double kMaxConvertibleMagnitude = 1e9; + class Tempo { public: - // The only place a bad BPM is rejected, which is what lets every conversion below be - // total — no resolver downstream needs a failure path. + // Rejects rather than clamps, alone among this module's doors: an unusable BPM has no + // nearest usable one to fall to. See this directory's CLAUDE.md for the rule. static std::optional fromBpm(double beatsPerMinute); double bpm() const { return bpm_; } diff --git a/tests/test_musical_division.cpp b/tests/test_musical_division.cpp index f9cc6b9..0997af2 100644 --- a/tests/test_musical_division.cpp +++ b/tests/test_musical_division.cpp @@ -3,7 +3,8 @@ // // Covers: the beat length of all 39 divisions against a literal rung table (NOT the module's // own exponent formula); the 1/64 and 64/1 extremes; the four named example divisions; the -// label notation; picker order and index round-trip; off-ladder clamping. +// label notation; picker order and index round-trip; off-ladder clamping of BOTH persisted +// fields, measured through the readers rather than by comparing two clamped values. #include "../src/core/instrument/note/musical_division.h" @@ -76,9 +77,14 @@ static void testExtremes() { 0.0625)); CHECK(almostEqual(divisionBeats(makeDivision(kMaxQuarterExponent, DivisionModifier::Straight)), 256.0)); - // The dotted 64/1 is the single longest programmable note. + // The dotted 64/1 is the single longest programmable note, and kMaxDivisionBeats — which + // note_program checks the tempo conversions' domain against — must name exactly it. CHECK(almostEqual(divisionBeats(makeDivision(kMaxQuarterExponent, DivisionModifier::Dotted)), 384.0)); + CHECK(almostEqual(kMaxDivisionBeats, 384.0)); + for (int i = 0; i < kDivisionCount; ++i) { + CHECK(divisionBeats(divisionAt(i)) <= kMaxDivisionBeats); + } // The 1/64 triplet is the shortest. CHECK(almostEqual(divisionBeats(makeDivision(kMinQuarterExponent, DivisionModifier::Triplet)), 0.0625 * 2.0 / 3.0)); @@ -144,24 +150,47 @@ static void testEverySetMemberIsDistinct() { // --- Clamping ----------------------------------------------------------------- static void testOffLadderExponentClampsToTheNearestRung() { - CHECK(makeDivision(-99, DivisionModifier::Straight) - == makeDivision(kMinQuarterExponent, DivisionModifier::Straight)); - CHECK(makeDivision(99, DivisionModifier::Triplet) - == makeDivision(kMaxQuarterExponent, DivisionModifier::Triplet)); - // A record carrying an off-ladder exponent still resolves to a real length. - Division corrupt; - corrupt.quarterExponent = 120; - CHECK(almostEqual(divisionBeats(corrupt), 256.0)); + // Asserted through the readers, never by comparing two clamped Divisions: a clamp that + // collapsed every exponent to one rung would make Division-to-Division comparisons agree + // with their own mistake. + CHECK(almostEqual(divisionBeats(makeDivision(-99, DivisionModifier::Straight)), 0.0625)); + CHECK(divisionLabel(makeDivision(-99, DivisionModifier::Straight)) == "1/64"); + CHECK(divisionIndex(makeDivision(-99, DivisionModifier::Straight)) == 0); + + CHECK(almostEqual(divisionBeats(makeDivision(99, DivisionModifier::Triplet)), + 256.0 * 2.0 / 3.0)); + CHECK(divisionLabel(makeDivision(99, DivisionModifier::Triplet)) == "64/1t"); + CHECK(divisionIndex(makeDivision(99, DivisionModifier::Triplet)) == kDivisionCount - 1); + + // The exponent that only a corrupt persisted record could carry still names a real rung. + CHECK(almostEqual(divisionBeats(makeDivision(120, DivisionModifier::Straight)), 256.0)); } -static void testEqualityNormalizesOffLadderExponentsLikeEveryOtherReader() { - // divisionBeats/divisionIndex/divisionLabel all re-clamp through makeDivision; equality - // must too, or a corrupt persisted value reads as a spurious diff on every reload. - Division corrupt; - corrupt.quarterExponent = 120; - corrupt.modifier = DivisionModifier::Straight; - CHECK(corrupt == makeDivision(kMaxQuarterExponent, DivisionModifier::Straight)); - CHECK(corrupt != makeDivision(kMaxQuarterExponent, DivisionModifier::Dotted)); +static void testUnnamedModifierClampsToStraight() { + // The other half of the persisted pair. Neither divisionBeats nor divisionLabel can see + // an unnamed modifier — both already fall through to the straight case — so the clamp is + // measured where it does show: the picker index and equality. + const DivisionModifier junk = static_cast(7); + CHECK(divisionIndex(makeDivision(0, junk)) + == 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)); + CHECK(makeDivision(0, junk) != makeDivision(0, DivisionModifier::Dotted)); +} + +static void testEveryConstructibleDivisionIndexesIntoThePickerSet() { + // divisionIndex is what a picker array is subscripted with, so an out-of-set index is an + // overrun in the caller. Both corrupt fields at once is the worst case: 12*3+7 without a + // modifier clamp. + const int exponents[] = {-9000, -99, kMinQuarterExponent, 0, kMaxQuarterExponent, 120, 9000}; + for (int e : exponents) { + for (int m = 0; m < 260; ++m) { + const Division d = makeDivision(e, static_cast(m)); + const int index = divisionIndex(d); + CHECK(index >= 0 && index < kDivisionCount); + CHECK(divisionAt(index) == d); // and the picker round-trips it back + } + } } static void testOutOfRangeIndexClampsIntoTheSet() { @@ -182,7 +211,8 @@ int main() { testEverySetMemberIsDistinct(); testOffLadderExponentClampsToTheNearestRung(); - testEqualityNormalizesOffLadderExponentsLikeEveryOtherReader(); + testUnnamedModifierClampsToStraight(); + testEveryConstructibleDivisionIndexesIntoThePickerSet(); testOutOfRangeIndexClampsIntoTheSet(); if (g_fail == 0) std::printf("musical_division: all tests passed\n"); diff --git a/tests/test_note_program.cpp b/tests/test_note_program.cpp index 92ca6c8..9d1c3f9 100644 --- a/tests/test_note_program.cpp +++ b/tests/test_note_program.cpp @@ -1,16 +1,20 @@ // Standalone tests for reasampler::instrument::note::note_program — no VST3, no REAPER, no // framework. Same fast assert loop as the sibling pure tests. // -// Covers: velocity clamping; the ms/beats denomination seam and its round-trip, including a -// corrupt denomination byte; anchoring (start to note-on, end to note-off); the resolved -// window against hand-computed values and its windowCollapsed flag; every division resolving -// to its duration in seconds; proportionality across two tempos; record equality and copy -// round-trip; editing an offset via its non-stored view (withMsView/withBeatsView). +// Covers: velocity clamping; the ms/beats denomination seam and its round-trip; anchoring +// (start to note-on, end to note-off); the resolved window against hand-computed values and +// its windowCollapsed flag, including the zero-length window the flag exists to distinguish; +// every division resolving to its duration in seconds; proportionality across two tempos; +// record equality and copy round-trip; editing an offset via its non-stored view +// (withMsView/withBeatsView); what `offsetOf` does to a corrupt magnitude or denomination, +// asserted through EVERY function that branches on one; and the module's headline claim — +// that resolveNote returns finite times for every constructible input. #include "../src/core/instrument/note/note_program.h" #include #include +#include using namespace reasampler::instrument::note; @@ -101,9 +105,9 @@ static void testRedenominationRoundTripsLosslessly() { const OffsetAmount original = offsetFromMs(ms); const OffsetAmount there = redenominate(original, Denomination::Beats, t); const OffsetAmount back = redenominate(there, Denomination::Milliseconds, t); - CHECK(there.denomination == Denomination::Beats); - CHECK(back.denomination == Denomination::Milliseconds); - CHECK(almostEqual(back.magnitude, ms, 1e-9 + 1e-9 * std::fabs(ms))); + CHECK(there.denomination() == Denomination::Beats); + CHECK(back.denomination() == Denomination::Milliseconds); + CHECK(almostEqual(back.magnitude(), ms, 1e-9 + 1e-9 * std::fabs(ms))); // Re-denominating never moves the instant it names. CHECK(almostEqual(offsetSeconds(there, t), offsetSeconds(original, t))); } @@ -112,7 +116,7 @@ static void testRedenominationRoundTripsLosslessly() { const OffsetAmount back = redenominate(redenominate(original, Denomination::Milliseconds, t), Denomination::Beats, t); - CHECK(almostEqual(back.magnitude, beats, 1e-9 + 1e-9 * std::fabs(beats))); + CHECK(almostEqual(back.magnitude(), beats, 1e-9 + 1e-9 * std::fabs(beats))); } } } @@ -327,19 +331,71 @@ static void testDefaultRecordIsAQuarterNoteWithNoOffsets() { CHECK(r.velocity == 100); // NoteProgram{}'s default Velocity, documented in note_program.h } -// --- Corrupt denomination byte -------------------------------------------------- +// --- The door: what a corrupt persisted field becomes ---------------------------- -static void testOutOfRangeDenominationReadsAsMillisecondsEverywhere() { - // A denomination byte outside {Milliseconds, Beats} is well-defined but unnamed; all - // three readers must default it to the same interpretation or a popup and a bake can - // report different instants for one record. - OffsetAmount corrupt; - corrupt.magnitude = 250.0; - corrupt.denomination = static_cast(7); +static void testUnnamedDenominationBecomesMilliseconds() { + // A denomination byte outside {Milliseconds, Beats} is well-defined but unnamed. The + // door pins it, so it is not merely that the readers agree — the value they read from + // is already Milliseconds by the time any of them sees it. + const OffsetAmount corrupt = offsetOf(250.0, static_cast(7)); + CHECK(corrupt.denomination() == Denomination::Milliseconds); const Tempo t = at(120.0); CHECK(almostEqual(offsetMs(corrupt, t), 250.0)); CHECK(almostEqual(offsetSeconds(corrupt, t), 0.25)); - CHECK(almostEqual(offsetBeats(corrupt, t), t.msToBeats(250.0))); + CHECK(almostEqual(offsetBeats(corrupt, t), 0.5)); +} + +static void testEveryDenominationBranchingFunctionAgreesWithThePin() { + // The pin is worth nothing if one branching function disagrees with it: an editor that + // flipped a corrupt record to beats would silently change whether it follows the tempo, + // and an equality that saw the raw byte would report a diff on every reload. All six. + const Tempo t = at(120.0); // one beat is 500 ms + const OffsetAmount corrupt = offsetOf(250.0, static_cast(7)); + const OffsetAmount asMs = offsetFromMs(250.0); + + CHECK(corrupt == asMs); // offsetMs / offsetBeats / offsetSeconds covered above + CHECK(!(corrupt != asMs)); + CHECK(redenominate(corrupt, Denomination::Milliseconds, t) == corrupt); + CHECK(redenominate(corrupt, Denomination::Beats, t).denomination() == Denomination::Beats); + CHECK(withMsView(corrupt, 40.0, t) == withMsView(asMs, 40.0, t)); + CHECK(withMsView(corrupt, 40.0, t).denomination() == Denomination::Milliseconds); + 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. + CHECK(redenominate(offsetFromBeats(1.0), static_cast(7), t) + == offsetFromMs(500.0)); +} + +static void testCorruptMagnitudeIsBoundedAtTheDoor() { + const double inf = std::numeric_limits::infinity(); + const double nan = std::numeric_limits::quiet_NaN(); + // NaN names no value to clamp toward, so it takes the field's own default; an infinity + // does have a nearest representable magnitude, so it clamps like any other overshoot. + CHECK(almostEqual(offsetFromMs(nan).magnitude(), 0.0)); + CHECK(almostEqual(offsetFromBeats(nan).magnitude(), 0.0)); + CHECK(almostEqual(offsetFromMs(inf).magnitude(), kMaxConvertibleMagnitude)); + CHECK(almostEqual(offsetFromBeats(-inf).magnitude(), -kMaxConvertibleMagnitude)); + CHECK(almostEqual(offsetFromMs(1e300).magnitude(), kMaxConvertibleMagnitude)); + // A NaN offset is a value, not a hole: it equals itself, so it is not a spurious diff. + CHECK(offsetFromMs(nan) == offsetFromMs(0.0)); + // Anything inside the domain passes through untouched. + CHECK(almostEqual(offsetFromMs(-12345.678).magnitude(), -12345.678)); +} + +static void testANanMagnitudeCannotReachTheResolvedWindow() { + // The witness the door exists for: at an unremarkable tempo, a NaN magnitude used to + // make captureStart, rawEnd and captureEnd all NaN, and windowCollapsed read false. + const Tempo t = at(120.0); + const double nan = std::numeric_limits::quiet_NaN(); + const ResolvedNote r = resolveNote( + program(makeDivision(0, DivisionModifier::Straight), offsetOf(nan, Denomination::Beats), + offsetOf(nan, Denomination::Milliseconds), 100), + t); + CHECK(almostEqual(r.captureStartSeconds, 0.0)); + CHECK(almostEqual(r.captureEndSeconds, 0.5)); + CHECK(!r.windowCollapsed); } // --- windowCollapsed ------------------------------------------------------------- @@ -359,18 +415,71 @@ static void testWindowCollapsedFlagsAnInvertedWindow() { CHECK(!normal.windowCollapsed); } +static void testWindowCollapsedIsFalseForAGenuinelyZeroLengthWindow() { + // The discrimination the flag exists for. A 1/4 at 120 BPM is 500 ms, so an end offset + // of -500 ms puts the raw end EXACTLY on the start: zero-length, but programmed that way + // rather than collapsed, and a popup must be able to tell the two apart. + const Tempo t = at(120.0); + const ResolvedNote r = resolveNote(program(makeDivision(0, DivisionModifier::Straight), + offsetFromMs(0.0), offsetFromMs(-500.0), 100), + t); + CHECK(almostEqual(r.captureLengthSeconds(), 0.0)); + CHECK(!r.windowCollapsed); + // One millisecond further in is the same zero length, but collapsed. + const ResolvedNote collapsed = resolveNote( + program(makeDivision(0, DivisionModifier::Straight), offsetFromMs(0.0), + offsetFromMs(-501.0), 100), + t); + CHECK(almostEqual(collapsed.captureLengthSeconds(), 0.0)); + CHECK(collapsed.windowCollapsed); +} + +// --- Totality -------------------------------------------------------------------- + +static void testResolveNoteIsFiniteForEveryConstructibleInput() { + // The claim that lets resolveNote have no failure path, swept rather than argued: every + // division, both denominations, the magnitude extremes the door admits plus the garbage + // it normalizes, across tempos from rejected-subnormal to rejected-astronomical. + const double inf = std::numeric_limits::infinity(); + const double nan = std::numeric_limits::quiet_NaN(); + 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}) { + const std::optional tempo = Tempo::fromBpm(bpm); + if (!tempo) { ++rejected; continue; } + ++accepted; + for (int i = 0; i < kDivisionCount; ++i) { + for (double m : magnitudes) { + for (Denomination d : {Denomination::Milliseconds, Denomination::Beats}) { + const ResolvedNote r = resolveNote( + program(divisionAt(i), offsetOf(m, d), offsetOf(-m, d), 100), *tempo); + CHECK(std::isfinite(r.noteOffSeconds)); + CHECK(std::isfinite(r.captureStartSeconds)); + CHECK(std::isfinite(r.captureEndSeconds)); + CHECK(std::isfinite(r.captureLengthSeconds())); + CHECK(r.captureLengthSeconds() >= 0.0); + } + } + } + } + // Neither half of the tempo sweep may be empty, or the loop above proves nothing. + CHECK(accepted > 0); + CHECK(rejected > 0); +} + // --- Editing via the non-stored view --------------------------------------------- static void testWithMsViewPreservesTheStoredDenomination() { const Tempo t = at(120.0); // one beat is 500 ms const OffsetAmount msOffset = offsetFromMs(10.0); const OffsetAmount editedMs = withMsView(msOffset, 40.0, t); - CHECK(editedMs.denomination == Denomination::Milliseconds); - CHECK(almostEqual(editedMs.magnitude, 40.0)); + CHECK(editedMs.denomination() == Denomination::Milliseconds); + CHECK(almostEqual(editedMs.magnitude(), 40.0)); const OffsetAmount beatsOffset = offsetFromBeats(1.0); const OffsetAmount editedBeats = withMsView(beatsOffset, 250.0, t); - CHECK(editedBeats.denomination == Denomination::Beats); // stays beats-denominated + CHECK(editedBeats.denomination() == Denomination::Beats); // stays beats-denominated CHECK(almostEqual(offsetMs(editedBeats, t), 250.0, 1e-6)); // but reads back as 250 ms } @@ -378,12 +487,12 @@ static void testWithBeatsViewPreservesTheStoredDenomination() { const Tempo t = at(120.0); // one beat is 500 ms const OffsetAmount beatsOffset = offsetFromBeats(0.5); const OffsetAmount editedBeats = withBeatsView(beatsOffset, 2.0, t); - CHECK(editedBeats.denomination == Denomination::Beats); - CHECK(almostEqual(editedBeats.magnitude, 2.0)); + CHECK(editedBeats.denomination() == Denomination::Beats); + CHECK(almostEqual(editedBeats.magnitude(), 2.0)); const OffsetAmount msOffset = offsetFromMs(100.0); const OffsetAmount editedMs = withBeatsView(msOffset, 1.0, t); - CHECK(editedMs.denomination == Denomination::Milliseconds); // stays ms-denominated + CHECK(editedMs.denomination() == Denomination::Milliseconds); // stays ms-denominated CHECK(almostEqual(offsetBeats(editedMs, t), 1.0)); // but reads back as 1 beat } @@ -414,13 +523,19 @@ int main() { testRedenominatedRecordDescribesTheSameWindow(); testDefaultRecordIsAQuarterNoteWithNoOffsets(); - testOutOfRangeDenominationReadsAsMillisecondsEverywhere(); + testUnnamedDenominationBecomesMilliseconds(); + testEveryDenominationBranchingFunctionAgreesWithThePin(); + testCorruptMagnitudeIsBoundedAtTheDoor(); + testANanMagnitudeCannotReachTheResolvedWindow(); testWindowCollapsedFlagsAnInvertedWindow(); + testWindowCollapsedIsFalseForAGenuinelyZeroLengthWindow(); testWithMsViewPreservesTheStoredDenomination(); testWithBeatsViewPreservesTheStoredDenomination(); + testResolveNoteIsFiniteForEveryConstructibleInput(); + if (g_fail == 0) std::printf("note_program: all tests passed\n"); else std::printf("note_program: %d FAILED\n", g_fail); return g_fail == 0 ? 0 : 1; diff --git a/tests/test_tempo.cpp b/tests/test_tempo.cpp index 28c2a0a..85de36c 100644 --- a/tests/test_tempo.cpp +++ b/tests/test_tempo.cpp @@ -2,9 +2,10 @@ // framework. Same fast assert loop as the sibling pure tests. // // Covers: BPM validation (the only rejection point, which is what makes the conversions -// total); seconds-per-beat at several tempos; beats<->seconds and beats<->ms round-trips -// across tempos and signs; the proportionality between two tempos, asserted as a ratio -// rather than against any fixed seconds value. +// total) including the tempos whose reciprocal is finite but whose conversions overflow; +// seconds-per-beat at several tempos; beats<->seconds and beats<->ms round-trips across +// tempos and signs; the proportionality between two tempos, asserted as a ratio rather than +// against any fixed seconds value. #include "../src/core/instrument/note/tempo.h" @@ -46,6 +47,44 @@ static void testUnusableBpmIsRejected() { CHECK(!Tempo::fromBpm(1e-310).has_value()); } +static void testBpmWhoseReciprocalIsFineButWhoseConversionsOverflowIsRejected() { + // The gap a guard on 60/bpm alone leaves open: the reciprocal is an ordinary finite + // double, and the multiply that follows it is what blows up. + CHECK(std::isfinite(60.0 / 1e-306)); + CHECK(!Tempo::fromBpm(1e-306).has_value()); + // The fast end fails in the other direction — the divide, not the multiply. + CHECK(!Tempo::fromBpm(1e308).has_value()); +} + +static void testTheGuardAdmitsEveryRealTempoAndFarBeyond() { + // The guard is structural, not musical, so it must not have narrowed onto the range of + // tempos anyone would type. The extremes here are orders of magnitude past that. + for (double bpm : {1e-200, 1e-6, 0.001, 1.0, 20.0, 120.0, 240.0, 960.0, 1e6, 1e100}) { + CHECK(Tempo::fromBpm(bpm).has_value()); + } +} + +static void testEveryAcceptedTempoConvertsTheWholeDomainFinitely() { + // What the guard is FOR: past it, no conversion of a magnitude the module admits can + // reach inf or NaN, in either unit or either direction. + int accepted = 0, rejected = 0; + for (double bpm : {1e-320, 1e-306, 1e-300, 1e-100, 1e-6, 0.5, 120.0, 1e6, 1e100, 1e250, + 1e308}) { + const std::optional t = Tempo::fromBpm(bpm); + if (!t) { ++rejected; continue; } + ++accepted; + for (double m : {-kMaxConvertibleMagnitude, -1.0, 0.0, 1.0, kMaxConvertibleMagnitude}) { + CHECK(std::isfinite(t->beatsToSeconds(m))); + CHECK(std::isfinite(t->beatsToMs(m))); + CHECK(std::isfinite(t->msToBeats(m))); + CHECK(std::isfinite(t->secondsToBeats(msToSeconds(m)))); + } + } + // Neither half of the sweep may be empty, or the loop above proves nothing. + CHECK(accepted > 0); + CHECK(rejected > 0); +} + // --- Conversions --------------------------------------------------------------- static void testSecondsPerBeatFollowsBpm() { @@ -121,6 +160,9 @@ static void testMillisecondsAreTempoFree() { int main() { testUsableBpmIsAccepted(); testUnusableBpmIsRejected(); + testBpmWhoseReciprocalIsFineButWhoseConversionsOverflowIsRejected(); + testTheGuardAdmitsEveryRealTempoAndFarBeyond(); + testEveryAcceptedTempoConvertsTheWholeDomainFinitely(); testSecondsPerBeatFollowsBpm(); testBeatsToSecondsAtAKnownTempo();