From fbe3d45ea6c381952195faf1ecf45ee687a5247e Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Wed, 30 Oct 2024 10:54:19 -0400 Subject: [PATCH] Replace `integer_quotient` with `unblock_int_div` (#315) Instead of `integer_quotient(a, b)`, we now always write `a / unblock_int_div(b)`. This gives us everything we used to have with `integer_quotient`, but with two big advantages: 1. The form for the units code becomes more similar to the non-units code that it replaces (i.e., we're using the `/` symbol). 2. We can now support _templated code_ that works for both integral and non-integral types: `a / unblock_int_div(b)` works equally well for, say, `b` with floating point rep. There's no longer any use case for `integer_quotient`. We therefore immediately deprecate it. We will keep it deprecated for the entirety of the 0.4.0 cycle, and remove it for 0.5.0. Fixes #253. --- au/code/au/quantity.hh | 58 ++++++++++++++++++++++++++++++++++--- au/code/au/quantity_test.cc | 20 ++++++++++--- docs/reference/quantity.md | 12 ++++---- 3 files changed, 76 insertions(+), 14 deletions(-) diff --git a/au/code/au/quantity.hh b/au/code/au/quantity.hh index 5019177e..bebc9245 100644 --- a/au/code/au/quantity.hh +++ b/au/code/au/quantity.hh @@ -393,7 +393,8 @@ class Quantity { constexpr bool are_units_quantity_equivalent = AreUnitsQuantityEquivalent::value; static_assert(are_units_quantity_equivalent || !uses_integer_division, - "Integer division forbidden: use integer_quotient() if you really want it"); + "Integer division forbidden: wrap denominator in `unblock_int_div()` if you " + "really want it"); } constexpr Quantity(Rep value) : value_{value} {} @@ -401,9 +402,56 @@ class Quantity { Rep value_{}; }; +//////////////////////////////////////////////////////////////////////////////////////////////////// +// Machinery to explicitly unblock integer division. +// +// Dividing by `unblock_int_div(x)` will allow integer division for any `x`. If the division would +// have been allowed anyway, then `unblock_int_div` is a no-op: this enables us to write templated +// code to handle template parameters that may or may not be integral. + +template +class AlwaysDivisibleQuantity; + +// Unblock integer divisoin for a `Quantity`. +template +constexpr AlwaysDivisibleQuantity unblock_int_div(Quantity q) { + return AlwaysDivisibleQuantity{q}; +} + +// Unblock integer division for any non-`Quantity` type. +template +constexpr AlwaysDivisibleQuantity, R> unblock_int_div(R x) { + return AlwaysDivisibleQuantity, R>{make_quantity>(x)}; +} + +template +class AlwaysDivisibleQuantity { + public: + // Divide a `Quantity` by this always-divisible quantity type. + template + friend constexpr auto operator/(Quantity q2, AlwaysDivisibleQuantity q) { + return make_quantity>(q2.in(U2{}) / q.q_.in(U{})); + } + + // Divide any non-`Quantity` by this always-divisible quantity type. + template + friend constexpr auto operator/(T x, AlwaysDivisibleQuantity q) { + return make_quantity>(x / q.q_.in(U{})); + } + + friend constexpr AlwaysDivisibleQuantity unblock_int_div(Quantity q); + friend constexpr AlwaysDivisibleQuantity, R> unblock_int_div(R x); + + private: + constexpr AlwaysDivisibleQuantity(Quantity q) : q_{q} {} + + Quantity q_; +}; + // Force integer division beteween two integer Quantities, in a callsite-obvious way. template -constexpr auto integer_quotient(Quantity q1, Quantity q2) { +[[deprecated("Replace `integer_quotient(a, b)` with `a / unblock_int_div(b)`")]] constexpr auto +integer_quotient(Quantity q1, Quantity q2) { static_assert(std::is_integral::value && std::is_integral::value, "integer_quotient() can only be called with integral Rep"); return make_quantity>(q1.in(U1{}) / q2.in(U2{})); @@ -411,7 +459,8 @@ constexpr auto integer_quotient(Quantity q1, Quantity q2) { // Force integer division beteween an integer Quantity and a raw number. template -constexpr auto integer_quotient(Quantity q, T x) { +[[deprecated("Replace `integer_quotient(a, b)` with `a / unblock_int_div(b)`")]] constexpr auto +integer_quotient(Quantity q, T x) { static_assert(std::is_integral::value && std::is_integral::value, "integer_quotient() can only be called with integral Rep"); return make_quantity(q.in(U{}) / x); @@ -419,7 +468,8 @@ constexpr auto integer_quotient(Quantity q, T x) { // Force integer division beteween a raw number and an integer Quantity. template -constexpr auto integer_quotient(T x, Quantity q) { +[[deprecated("Replace `integer_quotient(a, b)` with `a / unblock_int_div(b)`")]] constexpr auto +integer_quotient(T x, Quantity q) { static_assert(std::is_integral::value && std::is_integral::value, "integer_quotient() can only be called with integral Rep"); return make_quantity>(x / q.in(U{})); diff --git a/au/code/au/quantity_test.cc b/au/code/au/quantity_test.cc index 39d6d8ff..e9dec5cd 100644 --- a/au/code/au/quantity_test.cc +++ b/au/code/au/quantity_test.cc @@ -906,17 +906,29 @@ TEST(AreQuantityTypesEquivalent, RequiresSameRepAndEquivalentUnits) { EXPECT_TRUE((AreQuantityTypesEquivalent::value)); } -TEST(integer_quotient, EnablesIntegerDivision) { - constexpr auto dt = integer_quotient(meters(60), (miles / hour)(65)); +TEST(UnblockIntDiv, EnablesTruncatingIntegerDivisionIntoQuantity) { + constexpr auto dt = meters(60) / unblock_int_div((miles / hour)(65)); EXPECT_THAT(dt, QuantityEquivalent((hour * meters / mile)(0))); +} - constexpr auto x = integer_quotient(meters(60), 31); +TEST(UnblockIntDiv, EnablesDividingByRawInteger) { + constexpr auto x = meters(60) / unblock_int_div(31); EXPECT_THAT(x, SameTypeAndValue(meters(1))); +} - constexpr auto freq = integer_quotient(1000, minutes(300)); +TEST(UnblockIntDiv, EnablesTruncatingIntegerDivisionIntoRawInteger) { + constexpr auto freq = 1000 / unblock_int_div(minutes(300)); EXPECT_THAT(freq, SameTypeAndValue(inverse(minutes)(3))); } +TEST(UnblockIntDiv, IsNoOpForDivisionThatWouldBeAllowedAnyway) { + auto expect_unblock_int_div_is_no_op = [](auto n, auto d) { + EXPECT_THAT(n / unblock_int_div(d), SameTypeAndValue(n / d)); + }; + expect_unblock_int_div_is_no_op(meters(60), (miles / hour)(65.0)); + expect_unblock_int_div_is_no_op(1.23, minutes(4.56)); +} + TEST(Quantity, CanIntegerDivideQuantitiesOfQuantityEquivalentUnits) { constexpr auto ratio = meters(60) / meters(25); EXPECT_EQ(ratio, 2); diff --git a/docs/reference/quantity.md b/docs/reference/quantity.md index 576e4f02..7e93356d 100644 --- a/docs/reference/quantity.md +++ b/docs/reference/quantity.md @@ -473,29 +473,29 @@ number](../discussion/concepts/dimensionless.md#exact-cancellation). If either _input_ is a raw number, then it only affects the value, not the unit. It's equivalent to a `Quantity` whose unit is [a unitless unit](./unit.md#unitless-unit). -#### `integer_quotient()` +#### `unblock_int_div()` Experience has shown that raw integer division can be dangerous in a units library context. It conflicts with intuitions, and can produce code that is silently and grossly incorrect: see the [integer division section](../troubleshooting.md#integer-division-forbidden) of the troubleshooting guide for an example. -To use integer division, you must ask for it explicitly by name, with the `integer_quotient()` -function. +To use integer division, you must ask for it explicitly by name, by calling `unblock_int_div()` on +the denominator. -??? example "Using `integer_quotient()` to explicitly opt in to integer division" +??? example "Using `unblock_int_div()` to explicitly opt in to integer division" This will not work: ```cpp miles(125) / hours(2); - // ^--- Forbidden! Compiler error. + // ^--- Forbidden! Compiler error. ``` However, this will work just fine: ```cpp - integer_quotient(miles(125), hours(2)); + miles(125) / unblock_int_div(hours(2)); ``` It produces `(miles / hour)(62)`.