From bdfb5a88e6895a2ba7749621be5aea359a2407ac Mon Sep 17 00:00:00 2001 From: TayfurYldz Date: Thu, 17 Sep 2026 23:47:26 +0300 Subject: [PATCH 1/4] fix(util): handle INT64_MIN rational operations --- CHANGELOG.md | 4 +++ docs/spec/util/rational.md | 49 +++++++++++----------------- include/morph/util/rational.hpp | 57 +++++++++++++++++++++------------ tests/test_quantity.cpp | 10 +++--- tests/test_rational_checked.cpp | 30 +++++++++++++++++ 5 files changed, 93 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d31d268a..eb6ceafb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ API surface). ## [Unreleased] +### Fixed + +- **`Rational` no longer invokes undefined behaviour on `INT64_MIN`.** The whole-integer constructor now canonicalises like the other constructors, and unary negation, `abs`, `reciprocal`, and multiplication cross-cancellation remain defined even if the public numerator is manually set to `INT64_MIN`. Unrepresentable magnitude is clamped to `INT64_MAX`, matching the existing saturation policy. + ### Changed - **`LocalBackend::execute` no longer rescans the pending-completion list on diff --git a/docs/spec/util/rational.md b/docs/spec/util/rational.md index ebc4ac39..85fd7a4b 100644 --- a/docs/spec/util/rational.md +++ b/docs/spec/util/rational.md @@ -177,37 +177,24 @@ result and `llround` maps `(-2^63 - 0.5, -2^63]` onto it. On x86's 80-bit a poisoned `INT64_MIN` numerator); where `long double == double` the same literal rounds past the bound and is rejected by the plain `2^63` check. -**`INT64_MIN` negation hazards.** `INT64_MIN` (`-2^63`) has no positive -counterpart in `int64`, so every place that negates a component is a latent UB -site when that exact value reaches it: - -- **unary `operator-`** — `Rational{Numerator{-numerator}, ...}`: negating an - `INT64_MIN` numerator overflows. -- **`from`** — guards **only** `denominator == 0`; it does not screen - `INT64_MIN` components, so a hostile-but-nonzero `(INT64_MIN, …)` pair flows - straight into the canonicalising constructor. -- **`reciprocal`** — negates the numerator in the `numerator < 0` branch; - `INT64_MIN` there overflows. -- **Rendering** (`morph::units::detail::formatRationalDecimal`) — **not one of - these.** It takes the numerator's magnitude through `detail::absU64`, which - negates in unsigned arithmetic. Negating in `int64_t` there is UB that UBSan - catches, and it is reachable: the whole-integer - `Rational{value, DecimalPlaces{n}}` constructor does not canonicalise, so the - clamp never runs on that path, and `numerator` is public. -- **`canonicalise`** — **not one of these either.** It clamps an `INT64_MIN` - numerator to `-INT64_MAX` (with an `error`-level log, `reportClamp`) *before* - any sign flip, and computes the gcd through `detail::absU64`, which negates in - unsigned arithmetic. Since it is the shared sink for every constructor and - operator, a value that reaches it is safe. - -The wire codec (`setWire`) also defends independently: it maps an `INT64_MIN` -`num`/`den` to `-INT64_MAX` *before* constructing, so untrusted input never -reaches the trap value at all. - -The entry points that do **not** canonicalise are where the hazard remains — the -whole-integer `Rational{value, DecimalPlaces{n}}` constructor retains its -numerator verbatim, and `numerator` is a public member. A UB site reached that -way is a confirmed, not a hypothetical, shape. +**`INT64_MIN` handling.** `INT64_MIN` (`-2^63`) has no positive counterpart +in `int64`, so direct signed negation is undefined. The whole-integer +constructor therefore delegates to the canonicalising constructor and clamps +`INT64_MIN` to `-INT64_MAX`, matching the full constructor and wire path. + +The public `numerator` member can still be assigned `INT64_MIN` manually, so +operations that may observe such a value defend independently: unary negation, +`abs`, `reciprocal`, and multiplication overflow/cross-cancellation use either +an explicit saturating branch or `detail::absU64`, which computes magnitude in +unsigned arithmetic. These operations remain defined even for a manually +poisoned value; where the exact magnitude is unrepresentable they clamp to the +adjacent `INT64_MAX` magnitude and preserve the existing error/saturation +policy. + +The wire codec (`setWire`) likewise maps an `INT64_MIN` `num`/`den` to +`-INT64_MAX` before constructing, so untrusted input never reaches a signed +negation trap. + ### Checked arithmetic diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index b0d1705c..9e733c53 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -389,7 +389,7 @@ struct Rational { /// @param whole The integer value; stored as `whole/1`. /// @param wantedPrecision Decimal precision; clamped to [0, kMaxDecimalPlaces]. constexpr Rational(std::int64_t whole, DecimalPlaces wantedPrecision) noexcept - : numerator{whole}, decimalPlaces{detail::clampDecimalPlaces(wantedPrecision.value)} {} + : Rational{Numerator{whole}, Denominator{1}, wantedPrecision} {} /// @brief Constructs from explicit numerator/denominator, then canonicalises. /// @@ -483,10 +483,13 @@ struct Rational { /// @return The rounded floating-point reading. [[nodiscard]] double toDouble(std::uint32_t requestedDecimalPlaces) const noexcept; - /// @brief Negates. @note Negating a Rational built from `INT64_MIN` overflows. + /// @brief Negates, clamping an `INT64_MIN` numerator to `INT64_MAX`. /// @return The value with the numerator's sign flipped. [[nodiscard]] constexpr Rational operator-() const noexcept { - return Rational{Numerator{-numerator}, Denominator{denominator}, decimalPlaces}; + constexpr auto minValue = std::numeric_limits::min(); + constexpr auto maxValue = std::numeric_limits::max(); + auto const negated = numerator == minValue ? maxValue : -numerator; + return Rational{Numerator{negated}, Denominator{denominator}, decimalPlaces}; } /// @brief Multiplicative inverse. @@ -496,7 +499,10 @@ struct Rational { return std::unexpected(RationalError::DivisionByZero); } if (numerator < 0) { - return Rational{Numerator{-denominator}, Denominator{-numerator}, decimalPlaces}; + constexpr auto minValue = std::numeric_limits::min(); + constexpr auto maxValue = std::numeric_limits::max(); + auto const reciprocalDenominator = numerator == minValue ? maxValue : -numerator; + return Rational{Numerator{-denominator}, Denominator{reciprocalDenominator}, decimalPlaces}; } return Rational{Numerator{denominator}, Denominator{numerator}, decimalPlaces}; } @@ -828,14 +834,16 @@ struct Rational { /// @brief `operator*=`'s arithmetic, without the overflow check. /// @param rhs Value to multiply by. constexpr void mulAssignUnchecked(const Rational& rhs) noexcept { - auto const absoluteLeftNumerator = numerator < 0 ? -numerator : numerator; - auto const absoluteRightNumerator = rhs.numerator < 0 ? -rhs.numerator : rhs.numerator; - auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, rhs.denominator); - auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, denominator); - auto const reducedLeftNumerator = numerator / crossDivisorOne; - auto const reducedRightNumerator = rhs.numerator / crossDivisorTwo; - auto const reducedLeftDenominator = denominator / crossDivisorTwo; - auto const reducedRightDenominator = rhs.denominator / crossDivisorOne; + auto const absoluteLeftNumerator = detail::absU64(numerator); + auto const absoluteRightNumerator = detail::absU64(rhs.numerator); + auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, static_cast(rhs.denominator)); + auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, static_cast(denominator)); + auto const signedCrossDivisorOne = static_cast(crossDivisorOne); + auto const signedCrossDivisorTwo = static_cast(crossDivisorTwo); + auto const reducedLeftNumerator = numerator / signedCrossDivisorOne; + auto const reducedRightNumerator = rhs.numerator / signedCrossDivisorTwo; + auto const reducedLeftDenominator = denominator / signedCrossDivisorTwo; + auto const reducedRightDenominator = rhs.denominator / signedCrossDivisorOne; numerator = reducedLeftNumerator * reducedRightNumerator; denominator = reducedLeftDenominator * reducedRightDenominator; widenPrecisionTo(rhs.decimalPlaces); @@ -893,15 +901,17 @@ struct Rational { /// @param rhs The factor. /// @return `true` if the product cannot be represented. [[nodiscard]] constexpr bool mulWouldOverflow(const Rational& rhs) const noexcept { - auto const absoluteLeftNumerator = numerator < 0 ? -numerator : numerator; - auto const absoluteRightNumerator = rhs.numerator < 0 ? -rhs.numerator : rhs.numerator; - auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, rhs.denominator); - auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, denominator); + auto const absoluteLeftNumerator = detail::absU64(numerator); + auto const absoluteRightNumerator = detail::absU64(rhs.numerator); + auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, static_cast(rhs.denominator)); + auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, static_cast(denominator)); if (crossDivisorOne == 0 || crossDivisorTwo == 0) { return false; // a zero numerator: the product is zero } - return detail::mulOverflows(numerator / crossDivisorOne, rhs.numerator / crossDivisorTwo) || - detail::mulOverflows(denominator / crossDivisorTwo, rhs.denominator / crossDivisorOne); + auto const signedCrossDivisorOne = static_cast(crossDivisorOne); + auto const signedCrossDivisorTwo = static_cast(crossDivisorTwo); + return detail::mulOverflows(numerator / signedCrossDivisorOne, rhs.numerator / signedCrossDivisorTwo) || + detail::mulOverflows(denominator / signedCrossDivisorTwo, rhs.denominator / signedCrossDivisorOne); } private: @@ -968,9 +978,14 @@ static_assert(std::is_standard_layout_v); /// @param value Value to take the absolute value of. /// @return The non-negative value with the same magnitude. [[nodiscard]] constexpr Rational abs(const Rational& value) noexcept { - return value.numerator < 0 - ? Rational{Numerator{-value.numerator}, Denominator{value.denominator}, value.decimalPlaces} - : value; + if (value.numerator >= 0) { + return value; + } + constexpr auto maxValue = std::numeric_limits::max(); + auto const magnitude = detail::absU64(value.numerator); + auto const clampedMagnitude = magnitude > static_cast(maxValue) ? maxValue + : static_cast(magnitude); + return Rational{Numerator{clampedMagnitude}, Denominator{value.denominator}, value.decimalPlaces}; } /// @brief Rounds toward positive infinity. diff --git a/tests/test_quantity.cpp b/tests/test_quantity.cpp index 17671d52..a0ff6931 100644 --- a/tests/test_quantity.cpp +++ b/tests/test_quantity.cpp @@ -658,13 +658,13 @@ TEST_CASE("NamedQuantity slices to a plain Quantity", "[quantity]") { // // formatRationalDecimal negated the numerator with signed arithmetic, which is // UB for INT64_MIN -- confirmed by UBSan at quantity.hpp:101 before the fix. -// INT64_MIN reaches it because the whole-integer `Rational{value, DecimalPlaces}` -// constructor does not canonicalise (and `numerator` is a public member), so the -// clamp in canonicalise() never runs on this path. +// The ordinary constructors canonicalise INT64_MIN, but `numerator` remains a +// public member for aggregate-like use, so a caller can still create this state +// explicitly. The formatter must remain defined for that poisoned value. TEST_CASE("formatRationalDecimal: an un-canonicalised INT64_MIN numerator renders exactly", "[quantity][rational][morph496]") { - morph::math::Rational const value{std::numeric_limits::min(), morph::math::DecimalPlaces{0}}; - // Precondition: this constructor really does keep the trap value. + morph::math::Rational value{0, morph::math::DecimalPlaces{0}}; + value.numerator = std::numeric_limits::min(); REQUIRE(value.numerator == std::numeric_limits::min()); // Under -fsanitize=undefined this line was the UB report; the magnitude must // survive the unsigned negation intact rather than wrapping. diff --git a/tests/test_rational_checked.cpp b/tests/test_rational_checked.cpp index 25cc9ed0..f467b06c 100644 --- a/tests/test_rational_checked.cpp +++ b/tests/test_rational_checked.cpp @@ -359,6 +359,36 @@ TEST_CASE("An intermediate-only overflow saturates toward the true sign", "[rati CHECK((negLhs + negRhs).numerator == -kMax); } +TEST_CASE("Whole-integer INT64_MIN construction is canonicalised", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + const Rational value{kMin, DecimalPlaces{2}}; + CHECK(value.numerator == -kMax); + CHECK(value.denominator == 1); +} + +TEST_CASE("Public INT64_MIN numerator operations stay defined", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + Rational raw{0, DecimalPlaces{2}}; + raw.numerator = kMin; + + CHECK((-raw).numerator == kMax); + CHECK(abs(raw).numerator == kMax); + + const auto inverse = raw.reciprocal(); + REQUIRE(inverse.has_value()); + CHECK(inverse->numerator == -1); + CHECK(inverse->denominator == kMax); + + CHECK((raw * whole(2)).numerator == -kMax); + const auto checked = checkedMul(raw, whole(2)); + REQUIRE_FALSE(checked.has_value()); + CHECK(checked.error() == RationalError::Overflow); +} + TEST_CASE("A numerator of INT64_MIN is clamped, not undefined", "[rational][checked][saturate]") { std::vector logged; const morph::log::ScopedLoggerOverride capture{ From bc43fba077257490dd54ecc69deb30b2f8e7c60c Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Fri, 18 Sep 2026 06:49:27 +0200 Subject: [PATCH 2/4] style: clang-format the INT64_MIN abs() clamp `abs()`'s new saturating ternary was wrapped by hand and does not match `.clang-format`, so the whole-tree `clang-format` gate fails on it. CI pins clang-format to CLANG_VERSION 22 and checks every tracked `.hpp`/`.cpp`, not just the changed ones, so this blocks the PR regardless of its own content. Formatting only -- reflows one expression onto two lines. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DEyA1ak434CNthJK4vKkUY --- include/morph/util/rational.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index 9e733c53..8895d2e0 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -983,8 +983,8 @@ static_assert(std::is_standard_layout_v); } constexpr auto maxValue = std::numeric_limits::max(); auto const magnitude = detail::absU64(value.numerator); - auto const clampedMagnitude = magnitude > static_cast(maxValue) ? maxValue - : static_cast(magnitude); + auto const clampedMagnitude = + magnitude > static_cast(maxValue) ? maxValue : static_cast(magnitude); return Rational{Numerator{clampedMagnitude}, Denominator{value.denominator}, value.decimalPlaces}; } From 0fcd3f1b74e8e12efd211c94062cccb0493ad642 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Fri, 18 Sep 2026 10:28:35 +0200 Subject: [PATCH 3/4] fix(util): close the poisoned-denominator paths the first pass left open Addresses the review on morph#561. The first pass hardened a poisoned `INT64_MIN` *numerator* and left three sibling paths reachable through a poisoned `denominator`, which the same threat model admits since the members are public. - `reciprocal()` no longer sign-corrects by hand. It hands the inverted pair to the canonicalising constructor, which clamps an `INT64_MIN` component *before* moving a negative denominator's sign onto the numerator. The previous `-denominator` was undefined for a poisoned denominator -- the exact bug this PR exists to remove, in the function next door. - `mulAssignUnchecked` and `mulWouldOverflow` cross-cancelled through `static_cast(denominator)`. On master these were `std::gcd(int64, int64)`, which takes `|m|, |n|` internally, so the cast turned a negative denominator into a huge magnitude and a different gcd -- wrong arithmetic where master was right. Now `detail::absU64`. - `mulWouldOverflow` reported a reduced product of exactly `INT64_MIN` as representable, so `checkedMul` could return a success that `canonicalise` had already changed to `-INT64_MAX`. It is now an overflow. This needs no poisoned operand: `(-2^62) * 2` reaches it. - `checkedDiv` declines a divisor carrying a poisoned component, before forming the reciprocal. `reciprocal()` must clamp such a component, so what it returns is not that divisor's inverse and the product would be an *inexact* value handed back as a success -- which the exact-or-`Overflow` contract forbids. The zero divisor is still reported ahead of it. The review suggested instead carrying the unsigned `2^63` magnitude through division's cross-cancellation, so that `2 / (INT64_MIN/1)` returns its exact `-1/2^62`. Declined deliberately: that would make `checkedDiv` accept quotients `dividedBy` still saturates, breaking the documented guarantee that the operators and the checked forms never disagree about which quotients fit. The conservative answer costs nothing for any value the type can produce, since every constructor canonicalises. Six tests. Five are regression tests, each verified to fail against the unfixed implementation. The sixth -- the overflowing addition with a poisoned addend, which the review asked for -- **passes without the fix**: addition was not changed here, so it pins pre-existing saturation behaviour rather than this change. Kept for the coverage the review wanted, and labelled honestly rather than counted as a regression test. Documentation updated across `rational.hpp`, `rational.md`, `quantity.hpp` and `quantity_type.md`, which still described the pre-fix behaviour. Refs #537 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DEyA1ak434CNthJK4vKkUY --- CHANGELOG.md | 4 +- docs/spec/util/quantity_type.md | 11 +-- docs/spec/util/rational.md | 26 ++++-- include/morph/util/quantity.hpp | 9 ++- include/morph/util/rational.hpp | 91 +++++++++++++++++---- tests/test_rational_checked.cpp | 138 ++++++++++++++++++++++++++++++-- 6 files changed, 241 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb6ceafb..bffc2ca5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,9 @@ API surface). ### Fixed -- **`Rational` no longer invokes undefined behaviour on `INT64_MIN`.** The whole-integer constructor now canonicalises like the other constructors, and unary negation, `abs`, `reciprocal`, and multiplication cross-cancellation remain defined even if the public numerator is manually set to `INT64_MIN`. Unrepresentable magnitude is clamped to `INT64_MAX`, matching the existing saturation policy. +- **`Rational` no longer invokes undefined behaviour on `INT64_MIN`.** The whole-integer constructor now canonicalises like the other constructors, and unary negation, `abs`, `reciprocal`, and multiplication cross-cancellation remain defined even if the public `numerator` or `denominator` is manually set to `INT64_MIN`. Unrepresentable magnitude is clamped to `INT64_MAX`, matching the existing saturation policy. `reciprocal` hands the inverted pair to the canonicalising constructor rather than negating a component itself, so the clamp is reported through the usual `error` log instead of applied silently. +- **`checkedMul` no longer reports success for a product canonicalisation then changes.** A reduced product of exactly `INT64_MIN` fits an `int64_t` but is not a representable `Rational` component — `canonicalise` clamps it to `-INT64_MAX` — so `mulWouldOverflow` now reports it. `checkedMul(Rational{-2^62}, Rational{2})` returns `Overflow` where it previously returned a clamped value as a success; `operator*` saturates to the same `-INT64_MAX/1` it produced before, under the overflow log rather than the clamp log. +- **`checkedDiv` no longer absorbs an inexact reciprocal.** A divisor carrying a hand-poisoned `INT64_MIN` component has no representable inverse, so `checkedDiv` reports `Overflow` instead of returning the product of the clamped one as a success. `dividedBy`/`operator/` are unchanged and still saturate. ### Changed diff --git a/docs/spec/util/quantity_type.md b/docs/spec/util/quantity_type.md index 5056487d..78be56dc 100644 --- a/docs/spec/util/quantity_type.md +++ b/docs/spec/util/quantity_type.md @@ -256,11 +256,12 @@ a value reads identically everywhere. There is a single formatting path and in-code references to one are references to `std::formatter`. `formatRationalDecimal` takes the numerator's magnitude through -`math::detail::absU64`, in unsigned arithmetic. Negating in `int64_t` instead -is undefined for `INT64_MIN`, and that value is reachable here: the -whole-integer `Rational{value, DecimalPlaces{n}}` constructor does not -canonicalise, so the clamp that would otherwise remove the trap value never -runs. +`math::detail::absU64`, in unsigned arithmetic. It negated in `int64_t` until +morph#496, which is undefined for `INT64_MIN`. Since morph#537 every `Rational` +constructor canonicalises — including the whole-integer +`Rational{value, DecimalPlaces{n}}` one — so no *constructed* value carries the +trap value. `numerator` is still a public member, so one can be assigned +directly, and the formatter stays defined for that. **The decimal form.** `formatRationalDecimal` renders the exact `Rational` as a fixed decimal at its **runtime `DecimalPlaces`** and then trims trailing zeros diff --git a/docs/spec/util/rational.md b/docs/spec/util/rational.md index 85fd7a4b..4bf732e5 100644 --- a/docs/spec/util/rational.md +++ b/docs/spec/util/rational.md @@ -93,8 +93,8 @@ the canonical `(numerator, denominator)` pair and ignores `decimalPlaces`. |---|---|---| | `operator+`, `operator-`, `operator*` (plain `Rational` × `Rational`) | `Rational` | `noexcept`, return a bare `Rational` — no error channel. This means *representable* results never fail; it does **not** mean the operation cannot go wrong. Reduced int64 cross-terms exceeding ~2^63 **saturate** at `±INT64_MAX/1` and log at `error`; the result is clamped and inexact, and the return type does not say so (see [Overflow & value-range envelope](#overflow--value-range-envelope)). Reduce-before-multiply (Knuth 4.5.1) to extend safe int64 range. Cross-cancellation before multiplication. | | `operator/`, `dividedBy` (plain `Rational` ÷ `Rational`) | `expected` | `DivisionByZero` when divisor's numerator is zero — and **that is the only error it reports.** Implemented by multiplying `*this` by the reciprocal (`den/num`, sign carried onto the numerator), so it **also propagates `max` precision**, and so it **saturates on overflow exactly like `operator*`**: an out-of-envelope quotient clamps to `±INT64_MAX/1`, logs at `error`, and is still returned as a *successful* `expected`. Use `checkedDiv` to have that reported. | -| `operator-` (unary) | `Rational` | Negates numerator. Precision preserved. **Negating `INT64_MIN` overflows.** | -| `reciprocal` | `expected` | Multiplicative inverse. `DivisionByZero` when the value is zero. **Precision is the operand's own `decimalPlaces`, not `max`** (it is a unary operation with no second operand to widen against). | +| `operator-` (unary) | `Rational` | Negates numerator. Precision preserved. A hand-poisoned `INT64_MIN` numerator **clamps** to `INT64_MAX` rather than overflowing. | +| `reciprocal` | `expected` | Multiplicative inverse. `DivisionByZero` when the value is zero. **Precision is the operand's own `decimalPlaces`, not `max`** (it is a unary operation with no second operand to widen against). The inverted pair goes through the canonicalising constructor, which carries the sign and clamps a hand-poisoned `INT64_MIN` component; nothing is negated here. | | `operator+=`, `-=`, `*=` (in-place) | `Rational&` | Mutate `*this`, widen precision to `max`, canonicalise. | ## Overflow & value-range envelope @@ -253,7 +253,11 @@ there is no valid answer to inspect. `checkedMul` checks the *cross-cancelled* factors `operator*` actually multiplies, not the raw operands: cross-cancelling is what keeps most products in range, so checking beforehand would reject pairs that multiply perfectly -well (`INT64_MAX/2 * 2/1` reduces to `INT64_MAX/1`). +well (`INT64_MAX/2 * 2/1` reduces to `INT64_MAX/1`). A reduced product of +exactly `INT64_MIN` counts as an overflow even though it fits an `int64_t`: +`canonicalise` clamps such a component to `-INT64_MAX`, so reporting success +would hand back a value canonicalisation has already changed. `(-2^62) * 2` is +the shortest case, and needs no poisoned operand. `checkedDiv` is the division member of the family, and it exists because division was the one operation with no exact-or-nothing form: `dividedBy` @@ -262,7 +266,17 @@ checks *its* result is told a clamped quotient succeeded. `checkedDiv` is `checkedMul` against `rhs.reciprocal()` — the same operand pair `dividedBy` forms internally — and it folds both failure modes into the one channel: `DivisionByZero` propagated from `reciprocal`, `Overflow` from `checkedMul`. -`dividedBy` itself saturates: `Quantity` already folds a +It also reports `Overflow` for a divisor carrying a hand-poisoned `INT64_MIN` +component, *before* forming the reciprocal: that component's `2^63` magnitude +has no `int64` counterpart, so `reciprocal` returns a clamped value that is not +the divisor's inverse, and the product of it would be an inexact success. This +is conservative rather than exact — `2 / (INT64_MIN/1)` has the representable +exact quotient `-1/2^62` — and deliberately so: reaching it would require +carrying the unsigned magnitude through the cross-cancellation rather than going +through `reciprocal`, which would let `checkedDiv` accept quotients `dividedBy` +still saturates, breaking the one-set-of-predicates property below. No value the +type can construct is affected. +`dividedBy` itself is unchanged and still saturates: `Quantity` already folds a failed division to `nullopt` (`docs/spec/error_handling.md`), and making `/` the sole operation that refuses to saturate would impose "overflow is fatal" on every caller, in-tree and out. @@ -478,7 +492,7 @@ through `setWire`. | `checkedAdd(a, b)` | `constexpr expected noexcept` — exact sum, or `Overflow`. | | `checkedSub(a, b)` | `constexpr expected noexcept` — exact difference, or `Overflow`. | | `checkedMul(a, b)` | `constexpr expected noexcept` — exact product, or `Overflow`. | -| `checkedDiv(a, b)` | `constexpr expected noexcept` — exact quotient, or `DivisionByZero`, or `Overflow`. `checkedMul` against `b.reciprocal()`; the form `dividedBy`/`operator/` do not provide, since those saturate and report success. | +| `checkedDiv(a, b)` | `constexpr expected noexcept` — exact quotient, or `DivisionByZero`, or `Overflow` (including when `b` carries a poisoned `INT64_MIN` component, whose reciprocal is not representable). `checkedMul` against `b.reciprocal()`; the form `dividedBy`/`operator/` do not provide, since those saturate and report success. | | `setWire(Wire)` | `void noexcept` — rebuilds through the canonicalising constructor, clamping what it cannot represent and counting the clamp. | | `Wire::validate()` | `constexpr bool noexcept` — whether these raw values decode without being clamped. | | `WireClampScope` | Scoped observer: how many `Rational` values were clamped while decoding. | @@ -522,7 +536,7 @@ expected operator+(Left const&, Right const&) noexcept; | Rounding mode is a parameter, defaulting to half away from zero | **`RoundingMode{HalfAwayFromZero, HalfEven}`, `HalfAwayFromZero` default** | A mode had to become visible once rounding became a *storage* operation rather than an implementation detail of display. The default follows morph's own formatter rather than the standards' `HALF_EVEN`, because the point of rounding on the dispatch path is that the stored value equals the displayed one; a `HalfEven` default would break that for every tie until the formatter learned the same mode. | | No `checkedRound` | **`roundToDecimalPlaces` saturates and logs** | Consistent with `+`/`-`/`*`/`/`: the `checked*` family covers the operations a caller is likely to drive with unbounded inputs. Rounding a value already representable at the target scale — the overwhelmingly common case, and every integer — takes a fast path that cannot overflow at all. | | 128-bit cross-product comparison | **`detail::mulU64`** | Exact ordering over the full int64 range without overflow. Uses `unsigned __int128` when available (GCC/Clang), portable 32-bit limb decomposition on MSVC. | -| Negation limitation | **`INT64_MIN` overflows** | Documented limitation. The wire codec clamps `INT64_MIN` components away for untrusted input. | +| `INT64_MIN` is not a component | **Clamped to `-INT64_MAX`, with an `error` log** | `-INT64_MIN` is not representable, so canonicalising it is undefined. Every constructor canonicalises, so no constructed value carries it; the members are public, so the operations that could still observe one clamp instead of negating. The wire codec clamps it independently for untrusted input. | | `fromFloat` not `constexpr` | **Uses `std::llround` / `std::isfinite`** | These standard library functions are not `constexpr`. The `fromFloat` overloads are `inline` out-of-class, `noexcept` but not `constexpr`. | ## Payload shape tag diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index ea4497c9..39e7e98b 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -106,10 +106,11 @@ namespace detail { // invariant guarantees denominator > 0, so only the numerator carries sign. bool const negative = value.numerator < 0; // Negate in unsigned arithmetic: `-INT64_MIN` is undefined as a signed - // operation, and INT64_MIN reaches here through the whole-integer - // `Rational{value, DecimalPlaces{n}}` constructor, which does not - // canonicalise (and `numerator` is public). `absU64` is the shared helper - // that gets this right. + // operation. Every Rational constructor now canonicalises, so no + // constructed value carries INT64_MIN -- but `numerator` is a public + // member, so a caller can still assign one, and a formatter must stay + // defined for whatever it is handed. `absU64` is the shared helper that + // gets this right -- see morph#496 and morph#537. auto const num = ::morph::math::detail::absU64(value.numerator); auto const den = static_cast(value.denominator); auto const places = static_cast(value.decimalPlaces.value); diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index 8895d2e0..7f308d62 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -86,9 +86,14 @@ /// constructor, so a non-canonical payload (`1234/100`) or a hostile one /// (`den == 0`, out-of-range `dp`) always lands as a valid, reduced value. /// -/// @note Negating a `Rational` built from `INT64_MIN` overflows; avoid that -/// extreme value in code (the wire codec clamps it away for untrusted -/// input). +/// @note `INT64_MIN` is never a canonical component. It has no positive +/// counterpart in `int64`, so `canonicalise` -- which every constructor +/// runs, including the whole-integer one -- clamps such a component to +/// `-INT64_MAX` and logs at `error`. The members are public, so a caller +/// can still assign `INT64_MIN` by hand; the operations that may observe +/// one (unary `-`, `abs`, `reciprocal`, multiplication and its overflow +/// predicate) stay defined and clamp its magnitude rather than negating +/// it. The wire codec clamps it independently for untrusted input. /// @note Comparison is exact over the full int64 range (128-bit cross /// products). Arithmetic, however, has the usual fixed-width envelope: /// `+`, `-`, `*` overflow int64 when reduced cross terms exceed ~2^63 @@ -386,6 +391,10 @@ struct Rational { constexpr Rational() noexcept = default; /// @brief Constructs from a whole integer at the given precision. + /// + /// Delegates to the canonicalising constructor, so this path applies the + /// same clamps as every other: an `INT64_MIN` @p whole is stored as + /// `-INT64_MAX/1` with an `error` log rather than kept verbatim. /// @param whole The integer value; stored as `whole/1`. /// @param wantedPrecision Decimal precision; clamped to [0, kMaxDecimalPlaces]. constexpr Rational(std::int64_t whole, DecimalPlaces wantedPrecision) noexcept @@ -493,17 +502,22 @@ struct Rational { } /// @brief Multiplicative inverse. + /// + /// The inverted pair is handed straight to the canonicalising constructor + /// rather than sign-corrected here: `canonicalise` already moves a negative + /// denominator's sign onto the numerator, and it clamps an `INT64_MIN` + /// component (with an `error` log) *before* the flip. Negating either + /// component here instead would be undefined for a hand-poisoned + /// `INT64_MIN` in that position -- which is how this function reached + /// `-denominator` unguarded. + /// + /// A poisoned component therefore yields a clamped, inexact inverse. That + /// is the type's saturation policy, and `checkedDiv` declines to absorb it. /// @return `denominator/numerator`, or `unexpected(DivisionByZero)` if zero. [[nodiscard]] constexpr std::expected reciprocal() const noexcept { if (numerator == 0) { return std::unexpected(RationalError::DivisionByZero); } - if (numerator < 0) { - constexpr auto minValue = std::numeric_limits::min(); - constexpr auto maxValue = std::numeric_limits::max(); - auto const reciprocalDenominator = numerator == minValue ? maxValue : -numerator; - return Rational{Numerator{-denominator}, Denominator{reciprocalDenominator}, decimalPlaces}; - } return Rational{Numerator{denominator}, Denominator{numerator}, decimalPlaces}; } @@ -836,8 +850,8 @@ struct Rational { constexpr void mulAssignUnchecked(const Rational& rhs) noexcept { auto const absoluteLeftNumerator = detail::absU64(numerator); auto const absoluteRightNumerator = detail::absU64(rhs.numerator); - auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, static_cast(rhs.denominator)); - auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, static_cast(denominator)); + auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, detail::absU64(rhs.denominator)); + auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, detail::absU64(denominator)); auto const signedCrossDivisorOne = static_cast(crossDivisorOne); auto const signedCrossDivisorTwo = static_cast(crossDivisorTwo); auto const reducedLeftNumerator = numerator / signedCrossDivisorOne; @@ -898,20 +912,38 @@ struct Rational { /// Checks the cross-cancelled factors `operator*=` actually multiplies: /// cross-cancelling is what keeps most products in range, so checking the /// raw operands would reject pairs that multiply perfectly well. + /// + /// "Representable" is stricter than "fits an `int64_t`": `INT64_MIN` fits + /// and is still not a canonical component, because `canonicalise` clamps + /// it to `-INT64_MAX`. A product landing exactly there is reported here as + /// an overflow, so `checkedMul` cannot return a success that + /// canonicalisation has already altered -- `(-2^62) * 2` is the shortest + /// example, and needs no poisoned operand at all. /// @param rhs The factor. /// @return `true` if the product cannot be represented. [[nodiscard]] constexpr bool mulWouldOverflow(const Rational& rhs) const noexcept { auto const absoluteLeftNumerator = detail::absU64(numerator); auto const absoluteRightNumerator = detail::absU64(rhs.numerator); - auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, static_cast(rhs.denominator)); - auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, static_cast(denominator)); + auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, detail::absU64(rhs.denominator)); + auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, detail::absU64(denominator)); if (crossDivisorOne == 0 || crossDivisorTwo == 0) { return false; // a zero numerator: the product is zero } auto const signedCrossDivisorOne = static_cast(crossDivisorOne); auto const signedCrossDivisorTwo = static_cast(crossDivisorTwo); - return detail::mulOverflows(numerator / signedCrossDivisorOne, rhs.numerator / signedCrossDivisorTwo) || - detail::mulOverflows(denominator / signedCrossDivisorTwo, rhs.denominator / signedCrossDivisorOne); + auto const reducedLeftNumerator = numerator / signedCrossDivisorOne; + auto const reducedRightNumerator = rhs.numerator / signedCrossDivisorTwo; + auto const reducedLeftDenominator = denominator / signedCrossDivisorTwo; + auto const reducedRightDenominator = rhs.denominator / signedCrossDivisorOne; + if (detail::mulOverflows(reducedLeftNumerator, reducedRightNumerator) || + detail::mulOverflows(reducedLeftDenominator, reducedRightDenominator)) { + return true; + } + // Forming the products is safe now, and only now: the checks above + // have just proved neither overflows. + constexpr auto minValue = std::numeric_limits::min(); + return reducedLeftNumerator * reducedRightNumerator == minValue || + reducedLeftDenominator * reducedRightDenominator == minValue; } private: @@ -1165,13 +1197,40 @@ static_assert(std::is_standard_layout_v); /// predicate the operator uses. The operators and the checked forms therefore /// cannot disagree about which quotients fit. /// +/// A divisor carrying a hand-poisoned `INT64_MIN` component is reported as +/// `Overflow` before the reciprocal is formed. `reciprocal()` has to clamp such +/// a component's `2^63` magnitude to `INT64_MAX`, so the reciprocal it returns +/// is not the divisor's inverse, and multiplying by it would produce an +/// *inexact* value this function would otherwise hand back as a success. This +/// is the conservative side of the contract rather than the exact one: for some +/// dividends -- `2 / (INT64_MIN/1)`, whose exact quotient is `-1/2^62` -- the +/// answer is representable even though the reciprocal is not, and reaching it +/// would mean carrying the unsigned magnitude through the cross-cancellation +/// instead of going through `reciprocal()` at all. That would make `checkedDiv` +/// accept quotients `dividedBy` still saturates, which is the one thing the +/// paragraph above promises it will not do. It costs nothing for any value the +/// type can actually produce: every constructor canonicalises, so `INT64_MIN` +/// only ever reaches a component by direct assignment. +/// /// @param lhs Dividend. /// @param rhs Divisor. /// @return The exact quotient; `unexpected(RationalError::DivisionByZero)` if /// @p rhs is zero; `unexpected(RationalError::Overflow)` if the -/// quotient is not representable. +/// quotient, or the divisor's reciprocal, is not representable. [[nodiscard]] constexpr std::expected checkedDiv(const Rational& lhs, const Rational& rhs) noexcept { + // The zero divisor is reported ahead of everything else, as it is by + // `reciprocal()` itself. + if (rhs.numerator == 0) { + return std::unexpected(RationalError::DivisionByZero); + } + // A poisoned component makes `reciprocal()` clamp, so what it returns is + // not this divisor's inverse and the product below would be inexact. + // Decline rather than hand an inexact value back as a success. + constexpr auto minValue = std::numeric_limits::min(); + if (rhs.numerator == minValue || rhs.denominator == minValue) { + return std::unexpected(RationalError::Overflow); + } auto const divisorReciprocal = rhs.reciprocal(); if (!divisorReciprocal.has_value()) { return std::unexpected(divisorReciprocal.error()); diff --git a/tests/test_rational_checked.cpp b/tests/test_rational_checked.cpp index f467b06c..83592574 100644 --- a/tests/test_rational_checked.cpp +++ b/tests/test_rational_checked.cpp @@ -155,12 +155,11 @@ TEST_CASE("checkedSub mirrors checkedAdd", "[rational][checked]") { REQUIRE(ok.has_value()); CHECK(*ok == whole(2)); - // kMin + 1 (== -INT64_MAX), not kMin: constructing a Rational whose - // numerator is INT64_MIN is itself undefined behaviour -- canonicalise() - // negates the numerator unguarded, and -INT64_MIN is not representable. - // setWire guards that case on the wire path; the public constructor does - // not. Tracked separately; this test stays inside the representable range - // so it measures checkedSub rather than that. + // kMin + 1 (== -INT64_MAX), not kMin: `whole(kMin)` does not construct a + // Rational holding INT64_MIN. canonicalise() clamps such a numerator to + // -INT64_MAX and logs at error (morph#537), so passing kMin here would + // measure that clamp -- and the clamp's log -- rather than checkedSub. + // This test stays inside the representable range on purpose. // // -INT64_MAX - 2 is the first difference that genuinely does not fit // (-INT64_MAX - 1 is exactly INT64_MIN, which still does). @@ -389,6 +388,133 @@ TEST_CASE("Public INT64_MIN numerator operations stay defined", "[rational][chec CHECK(checked.error() == RationalError::Overflow); } +// Cross-cancellation takes both gcd operands as *magnitudes*. Reinterpreting a +// negative denominator as uint64_t instead yields 2^64 - |d|, whose gcd with +// the other numerator is a different number -- and one that need not divide the +// denominator at all, so the reduction truncates and the value changes. +// 1/-4 * 3/1 is the shortest witness: gcd(3, 4) == 1, but +// gcd(3, 2^64 - 4) == 3, which divides 3 and not -4. A negative denominator is +// a poisoned state for the same reason an INT64_MIN numerator is -- the members +// are public -- and master handled it correctly, so this guards a regression +// rather than a gap. +TEST_CASE("Multiplication cross-cancels on magnitudes, not a reinterpreted denominator", + "[rational][checked][saturate]") { + Rational poisoned{0, DecimalPlaces{2}}; + poisoned.numerator = 1; + poisoned.denominator = -4; + + const auto product = poisoned * whole(3); + CHECK(product.numerator == -3); + CHECK(product.denominator == 4); + + const auto checked = checkedMul(poisoned, whole(3)); + REQUIRE(checked.has_value()); + CHECK(checked->numerator == -3); + CHECK(checked->denominator == 4); +} + +// reciprocal() used to negate `denominator` directly, which is the very UB this +// change exists to remove -- just in the other component. The value here is +// -1/-2^63, a hair above zero, so its inverse is 2^63 and clamps to INT64_MAX. +// A signed negation wraps to INT64_MIN instead and lands on -INT64_MAX: the +// wrong sign, not merely the wrong magnitude. +TEST_CASE("reciprocal is defined for a poisoned INT64_MIN denominator", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + Rational raw{0, DecimalPlaces{2}}; + raw.numerator = -1; + raw.denominator = kMin; + + const auto inverse = raw.reciprocal(); + REQUIRE(inverse.has_value()); + CHECK(inverse->numerator == kMax); + CHECK(inverse->denominator == 1); +} + +// compareForSaturation asks `*this <=> -rhs`, so an overflowing addition puts +// the poisoned operand through unary negation. Under a signed negation that +// wraps to INT64_MIN the comparison inverts and the sum saturates toward +// +INT64_MAX -- the opposite end from the true sum. +TEST_CASE("An overflowing addition with a poisoned addend saturates toward the true sign", + "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + Rational raw{0, DecimalPlaces{2}}; + raw.numerator = kMin; + + const auto sum = whole(-1) + raw; + CHECK(sum.numerator == -kMax); + CHECK(sum.denominator == 1); +} + +// checkedMul promises exact-or-nothing, and INT64_MIN is not an exactness this +// type can keep: canonicalise() clamps such a component to -INT64_MAX. The +// numerator case needs no poisoned operand at all -- (-2^62) * 2 is two +// ordinary canonical values whose product is exactly INT64_MIN. +TEST_CASE("checkedMul reports a reduced product of exactly INT64_MIN as overflow", "[rational][checked]") { + constexpr auto halfMin = kMin / 2; // -2^62, perfectly representable + + const auto numeratorSide = checkedMul(whole(halfMin), whole(2)); + REQUIRE_FALSE(numeratorSide.has_value()); + CHECK(numeratorSide.error() == RationalError::Overflow); + + // The denominator side of the same rule. A negative denominator is a + // poisoned state, so this one has to be built by hand. + Rational poisoned{0, DecimalPlaces{2}}; + poisoned.numerator = 1; + poisoned.denominator = halfMin; + const Rational half{Numerator{1}, Denominator{2}, DecimalPlaces{2}}; + + const auto denominatorSide = checkedMul(poisoned, half); + REQUIRE_FALSE(denominatorSide.has_value()); + CHECK(denominatorSide.error() == RationalError::Overflow); +} + +TEST_CASE("checkedMul reports a poisoned INT64_MIN numerator times one as overflow", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + Rational raw{0, DecimalPlaces{2}}; + raw.numerator = kMin; + + // mulOverflows(INT64_MIN, 1) is correctly false -- the product fits an + // int64_t. It is canonicalisation that changes it afterwards, which is why + // the predicate has to know about representability and not just width. + const auto product = checkedMul(raw, whole(1)); + REQUIRE_FALSE(product.has_value()); + CHECK(product.error() == RationalError::Overflow); +} + +// reciprocal() has to clamp a poisoned component's 2^63 magnitude, so the value +// it returns is not the divisor's inverse and multiplying by it is inexact. +// checkedDiv is the exact-or-nothing form, so it declines instead of handing +// that back as a success. Deliberately conservative: 2 / (INT64_MIN/1) has the +// representable exact quotient -1/2^62. See checkedDiv's own doc comment. +TEST_CASE("checkedDiv declines a divisor whose reciprocal is not representable", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + Rational poisonedNumerator{0, DecimalPlaces{2}}; + poisonedNumerator.numerator = kMin; + const auto byNumerator = checkedDiv(whole(2), poisonedNumerator); + REQUIRE_FALSE(byNumerator.has_value()); + CHECK(byNumerator.error() == RationalError::Overflow); + + Rational poisonedDenominator{0, DecimalPlaces{2}}; + poisonedDenominator.numerator = -1; + poisonedDenominator.denominator = kMin; + const auto byDenominator = checkedDiv(whole(2), poisonedDenominator); + REQUIRE_FALSE(byDenominator.has_value()); + CHECK(byDenominator.error() == RationalError::Overflow); + + // Still reports the zero divisor ahead of everything else. + const auto byZero = checkedDiv(whole(2), whole(0)); + REQUIRE_FALSE(byZero.has_value()); + CHECK(byZero.error() == RationalError::DivisionByZero); +} + TEST_CASE("A numerator of INT64_MIN is clamped, not undefined", "[rational][checked][saturate]") { std::vector logged; const morph::log::ScopedLoggerOverride capture{ From 31675fd45ded6f70112b09d6a264f03d0d3dc252 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Fri, 18 Sep 2026 10:43:32 +0200 Subject: [PATCH 4/4] coverage: re-pin rational.hpp's branch-partial allowlist after the line shifts The eight `rational.hpp` entries all named lines that had moved, and the two `if (!std::is_constant_evaluated())` entries had become mutually ambiguous -- the checker disambiguates identical source text by the recorded line, so stale hints make it unable to tell them apart. Re-pinned to 276/717/751/777/795/929/ 1481/1572, and the three in-prose cross references in the reason strings now name the new lines too. No entry was added, removed, or had its reason changed: this is the mechanical consequence of the edits above it in the file. Refs #537 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DEyA1ak434CNthJK4vKkUY --- scripts/branch_partial_allowlist.json | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index d54bfc85..80fe9853 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -41,23 +41,23 @@ "entries": [ { "file": "include/morph/util/rational.hpp", - "line": 271, + "line": 276, "source": "assert(rawDecimalPlaces <= kMaxDecimalPlaces);", "reason": "assert()'s false arm calls abort(). Reaching it means violating the precondition and terminating the process, so no test in a Catch2 binary can observe it and still report; a death test would prove only that assert() works. The decision the assert guards is checked by the caller-side tests that stay inside kMaxDecimalPlaces." }, { "file": "include/morph/util/rational.hpp", - "line": 697, + "line": 717, "source": "if (!std::is_constant_evaluated()) {", "context": "/// @brief Logs an overflow that `operator+=`/`-=`/`*=` saturated instead", "reason": "The false arm is the constant-evaluated one, and constant evaluation does not execute instrumented code -- no counter can increment during it. So in every run this branch is true, structurally, and its other arm is not a gap in the tests but a gap in what runtime instrumentation can see. reportOverflow's constexpr behaviour is exercised by the constexpr Rational arithmetic in tests/test_rational.cpp; what cannot be exercised is llvm-cov's counter." }, { "file": "include/morph/util/rational.hpp", - "line": 731, + "line": 751, "source": "if (!std::is_constant_evaluated()) {", "context": "[[nodiscard]] constexpr int compareForSaturation(const Rational& rhs, bool addition) const noexcept {", - "reason": "reportClamp's copy of the same shape as line 697 above, and uncoverable for the same reason: the untaken arm is constant evaluation, which increments no counters." + "reason": "reportClamp's copy of the same shape as line 717 above, and uncoverable for the same reason: the untaken arm is constant evaluation, which increments no counters." }, { "file": "include/morph/util/datetime.hpp", @@ -67,31 +67,31 @@ }, { "file": "include/morph/util/rational.hpp", - "line": 757, + "line": 777, "source": "return std::is_gt(ordering) ? 1 : 0;", - "reason": "The false arm (a tie, returning 0) is unreachable. compareForSaturation has exactly two call sites, both from the saturating branch of operator+=/operator-=, and both are guarded by addWouldOverflow/subWouldOverflow having just returned true. A tie in compareForSaturation (*this == -rhs for +=, or *this == rhs for -=) requires -- since operator== compares canonical numerator/denominator pairs directly -- identical denominators. With equal denominators, addWouldOverflow/subWouldOverflow's own scaling factors collapse to rightScaled == leftScaled == 1 (gcd(D, D) == D), so the cross-multiplication checks reduce to numerator +/- rhs.numerator directly on already-canonical (so already in-range) int64_t values whose sum/difference is provably 0 -- never an overflow (and canonicalise() already forbids either component from being INT64_MIN, so negating one to check the other is always representable). So whenever the two operands could produce a tie, the overflow predicate that gates the call to compareForSaturation is always false, and the saturating path -- hence compareForSaturation's tie return -- is never reached with a tied pair. Shares this root cause with saturateToward's sign == 0 arm at line 775 below." + "reason": "The false arm (a tie, returning 0) is unreachable. compareForSaturation has exactly two call sites, both from the saturating branch of operator+=/operator-=, and both are guarded by addWouldOverflow/subWouldOverflow having just returned true. A tie in compareForSaturation (*this == -rhs for +=, or *this == rhs for -=) requires -- since operator== compares canonical numerator/denominator pairs directly -- identical denominators. With equal denominators, addWouldOverflow/subWouldOverflow's own scaling factors collapse to rightScaled == leftScaled == 1 (gcd(D, D) == D), so the cross-multiplication checks reduce to numerator +/- rhs.numerator directly on already-canonical (so already in-range) int64_t values whose sum/difference is provably 0 -- never an overflow (and canonicalise() already forbids either component from being INT64_MIN, so negating one to check the other is always representable). So whenever the two operands could produce a tie, the overflow predicate that gates the call to compareForSaturation is always false, and the saturating path -- hence compareForSaturation's tie return -- is never reached with a tied pair. Shares this root cause with saturateToward's sign == 0 arm at line 795 below." }, { "file": "include/morph/util/rational.hpp", - "line": 775, + "line": 795, "source": "numerator = sign == 0 ? 0 : (sign < 0 ? -maxValue : maxValue);", - "reason": "The sign == 0 arm is unreachable, for the same root cause as compareForSaturation's tie return at line 757 above: saturateToward is only ever called with the sign compareForSaturation returned, and compareForSaturation can only return 0 (a tie) for a pair whose overflow predicate is provably false -- so the saturating path that calls saturateToward is never entered with a tied pair, and sign is never 0 when this line runs." + "reason": "The sign == 0 arm is unreachable, for the same root cause as compareForSaturation's tie return at line 777 above: saturateToward is only ever called with the sign compareForSaturation returned, and compareForSaturation can only return 0 (a tie) for a pair whose overflow predicate is provably false -- so the saturating path that calls saturateToward is never entered with a tied pair, and sign is never 0 when this line runs." }, { "file": "include/morph/util/rational.hpp", - "line": 900, + "line": 929, "source": "if (crossDivisorOne == 0 || crossDivisorTwo == 0) {", "reason": "Both disjuncts are unreachable, and the body they guard (the zero-numerator short-circuit `return false;` right below) is correspondingly dead. crossDivisorOne = gcd(|numerator|, rhs.denominator), crossDivisorTwo = gcd(|rhs.numerator|, denominator). std::gcd(a, b) == 0 iff both a == 0 and b == 0. denominator/rhs.denominator can never be 0: every constructor path (Rational(Numerator, Denominator, DecimalPlaces)) calls canonicalise(), which clamps a 0 denominator to 1 before returning, and every mutating operation either recomputes the denominator as a product of positive denominators or calls canonicalise() again. So denominator > 0 (and rhs.denominator > 0) is a whole-class invariant, making crossDivisorOne/crossDivisorTwo == 0 impossible regardless of numerator/rhs.numerator." }, { "file": "include/morph/util/rational.hpp", - "line": 1498, + "line": 1572, "source": "if (ctx.begin() == ctx.end() || *ctx.begin() == '}') {", "reason": "The ctx.begin() == ctx.end() true arm is unreachable. This is the textbook cppreference-style custom-formatter parse() idiom. Empirically verified on this toolchain (libc++, via a standalone probe compiled with clang++ -std=c++23 -stdlib=libc++): for both std::format(\"{}\", x) and std::format(\"{:}\", x), ctx.end() always points past the terminating '}', so ctx.begin() == ctx.end() is false and the terminator is always reachable via *ctx.begin() == '}' -- matching the observed 0/14 split exactly. std::format's top-level parser (and std::vformat's, which performs the same replacement-field validation before dispatching to a type's parse()) rejects an unterminated '{...' before ever calling into formatter::parse, so this function is never invoked with an already-exhausted range. The first disjunct is defensive boilerplate that this standard library's implementation (and the standard's own guarantee about validated replacement fields) makes structurally unreachable." }, { "file": "include/morph/util/rational.hpp", - "line": 1407, + "line": 1481, "source": "if (!detail::addOverflows(whole, step)) {", "reason": "The false arm (the overflow-would-occur case, declining to step) is unreachable, contrary to Task 2's initial classification of this line as testable -- verified both mathematically and empirically (a standalone probe sweeping denominators/precisions found 476 cases where the scale-up saturated to whole == INT64_MAX, and every one had a fractional remainder of exactly 0, never >= 0.5) before writing this entry. A Rational's magnitude can never exceed INT64_MAX: its value is numerator/denominator with denominator >= 1 and numerator in [-INT64_MAX, INT64_MAX] (canonicalise() clamps INT64_MIN away), so |value| <= |numerator| <= INT64_MAX always. `whole` is trunc(scaled), so whole == INT64_MAX forces scaled == INT64_MAX/1 exactly -- there is no room for scaled to be in (INT64_MAX, INT64_MAX + 1) the way an unbounded rational could land. With scaled == whole exactly, `fraction` (scaled minus whole) is always 0, so `roundAway` (set from comparing fraction against 1/2) is always false when whole == INT64_MAX, and stepping never happens on that side. On the other side, step == -1 would need whole == INT64_MIN to overflow, but whole's range is [-INT64_MAX, INT64_MAX] (INT64_MIN is never a valid Rational magnitude), so that direction cannot overflow either. The guard is defensive: reachable only if a future change let a Rational's magnitude exceed INT64_MAX, which the type's invariants currently forbid everywhere else in this file." },