From 3abbb593c7dc76e01d2ce0190be6a44c35f9fffc Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Tue, 12 Dec 2023 20:43:11 -0500 Subject: [PATCH] Check that every magnitude fits in its target type `get_value(m)` converts a magnitude `m` to its representation in type `T`. It produces a hard compiler error if the value can't fit. Its relative `get_value_result(m)` always returns a result, but produces an error code if the value can't be represented. Previously, there was a hole in this system. We assumed that each magnitude could be represented in `std::uintmax_t`. Of course, if it couldn't, `get_value` would fail to compile as it should. But `get_value_result` would _also_ fail to compile, instead of producing an error condition. To fix this, we need to move our checking utilities further up in the file, so that all of our utilities can use them. `product`, `int_pow`, `base_power_value` --- they all need to _check their computations_ at every step. When we do this, we're able to gracefully detect the non-representability of huge numbers even for the biggest types. --- au/magnitude.hh | 89 ++++++++++++++++++++++++++++++++------------ au/magnitude_test.cc | 37 ++++++++++++++++++ 2 files changed, 103 insertions(+), 23 deletions(-) diff --git a/au/magnitude.hh b/au/magnitude.hh index 02eaaa45..e7cd880e 100644 --- a/au/magnitude.hh +++ b/au/magnitude.hh @@ -267,6 +267,21 @@ struct NumeratorImpl> namespace detail { +enum class MagRepresentationOutcome { + OK, + ERR_NON_INTEGER_IN_INTEGER_TYPE, + ERR_RATIONAL_POWERS, + ERR_CANNOT_FIT, +}; + +template +struct MagRepresentationOrError { + MagRepresentationOutcome outcome; + + // Only valid/meaningful if `outcome` is `OK`. + T value = {0}; +}; + // The widest arithmetic type in the same category. // // Used for intermediate computations. @@ -278,19 +293,61 @@ using Widen = std::conditional_t< std::conditional_t::value, std::intmax_t, std::uintmax_t>>, T>; +template +constexpr MagRepresentationOrError checked_int_pow(T base, std::uintmax_t exp) { + MagRepresentationOrError result = {MagRepresentationOutcome::OK, T{1}}; + while (exp > 0u) { + if (exp % 2u == 1u) { + if (base > std::numeric_limits::max() / result.value) { + return MagRepresentationOrError{MagRepresentationOutcome::ERR_CANNOT_FIT}; + } + result.value *= base; + } + + exp /= 2u; + + if (base > std::numeric_limits::max() / base) { + return (exp == 0u) + ? result + : MagRepresentationOrError{MagRepresentationOutcome::ERR_CANNOT_FIT}; + } + base *= base; + } + return result; +} + template -constexpr Widen base_power_value(B base) { - return (N < 0) ? (Widen{1} / base_power_value(base)) - : int_pow(static_cast>(base), static_cast(N)); +constexpr MagRepresentationOrError> base_power_value(B base) { + if (N < 0) { + const auto inverse_result = base_power_value(base); + if (inverse_result.outcome != MagRepresentationOutcome::OK) { + return inverse_result; + } + return { + MagRepresentationOutcome::OK, + Widen{1} / inverse_result.value, + }; + } + + return checked_int_pow(static_cast>(base), static_cast(N)); } template -constexpr T product(const T (&values)[N]) { +constexpr MagRepresentationOrError product(const MagRepresentationOrError (&values)[N]) { + for (const auto &x : values) { + if (x.outcome != MagRepresentationOutcome::OK) { + return x; + } + } + T result{1}; for (const auto &x : values) { - result *= x; + if ((x.value > 1) && (result > std::numeric_limits::max() / x.value)) { + return {MagRepresentationOutcome::ERR_CANNOT_FIT}; + } + result *= x.value; } - return result; + return {MagRepresentationOutcome::OK, result}; } template @@ -327,21 +384,6 @@ constexpr bool safe_to_cast_to(InputT x) { return SafeCastingChecker{}(x); } -enum class MagRepresentationOutcome { - OK, - ERR_NON_INTEGER_IN_INTEGER_TYPE, - ERR_RATIONAL_POWERS, - ERR_CANNOT_FIT, -}; - -template -struct MagRepresentationOrError { - MagRepresentationOutcome outcome; - - // Only valid/meaningful if `outcome` is `OK`. - T value = {0}; -}; - template constexpr MagRepresentationOrError get_value_result(Magnitude) { // Representing non-integer values in integral types is something we never plan to support. @@ -361,11 +403,12 @@ constexpr MagRepresentationOrError get_value_result(Magnitude) { constexpr auto widened_result = product({base_power_value::num / ExpT::den)>(BaseT::value())...}); - if (!safe_to_cast_to(widened_result)) { + if ((widened_result.outcome != MagRepresentationOutcome::OK) || + !safe_to_cast_to(widened_result.value)) { return {MagRepresentationOutcome::ERR_CANNOT_FIT}; } - return {MagRepresentationOutcome::OK, static_cast(widened_result)}; + return {MagRepresentationOutcome::OK, static_cast(widened_result.value)}; } // This simple overload avoids edge cases with creating and passing zero-sized arrays. diff --git a/au/magnitude_test.cc b/au/magnitude_test.cc index a74f2679..5e72a101 100644 --- a/au/magnitude_test.cc +++ b/au/magnitude_test.cc @@ -19,6 +19,7 @@ #include "au/testing.hh" #include "gtest/gtest.h" +using ::testing::DoubleEq; using ::testing::StaticAssertTypeEq; namespace au { @@ -265,6 +266,42 @@ TEST(CommonMagnitude, ZeroResultIndicatesAllInputsAreZero) { namespace detail { +MATCHER(CannotFit, "") { + return (arg.outcome == MagRepresentationOutcome::ERR_CANNOT_FIT) && (arg.value == 0); +} + +template +auto FitsAndMatchesValue(ValueMatcher &&matcher) { + return ::testing::AllOf( + ::testing::Field(&MagRepresentationOrError::outcome, + ::testing::Eq(MagRepresentationOutcome::OK)), + ::testing::Field(&MagRepresentationOrError::value, std::forward(matcher))); +} + +template +auto FitsAndProducesValue(T val) { + return FitsAndMatchesValue(SameTypeAndValue(val)); +} + +TEST(CheckedIntPow, FindsAppropriateLimits) { + EXPECT_THAT(checked_int_pow(int16_t{2}, 14), FitsAndProducesValue(int16_t{16384})); + EXPECT_THAT(checked_int_pow(int16_t{2}, 15), CannotFit()); + + EXPECT_THAT(checked_int_pow(uint16_t{2}, 15), FitsAndProducesValue(uint16_t{32768})); + EXPECT_THAT(checked_int_pow(uint16_t{2}, 16), CannotFit()); + + EXPECT_THAT(checked_int_pow(uint64_t{2}, 63), + FitsAndProducesValue(uint64_t{9'223'372'036'854'775'808u})); + EXPECT_THAT(checked_int_pow(uint64_t{2}, 64), CannotFit()); + + EXPECT_THAT(checked_int_pow(10.0, 308), FitsAndMatchesValue(DoubleEq(1e308))); + EXPECT_THAT(checked_int_pow(10.0, 309), CannotFit()); +} + +TEST(GetValueResult, HandlesNumbersTooBigForUintmax) { + EXPECT_THAT(get_value_result(pow<64>(mag<2>())), CannotFit()); +} + TEST(PrimeFactorizationT, NullMagnitudeFor1) { StaticAssertTypeEq, Magnitude<>>(); }