From 30440ed679c98127785c89398af9717382f230dd Mon Sep 17 00:00:00 2001
From: Chip Hogg <chogg@aurora.tech>
Date: Thu, 2 May 2024 10:15:25 -0400
Subject: [PATCH] Explicitly specify `QuantityPoint` calc type (#234)

We want to carry out our intermediate computations in the common type of
the old and new rep, generally.  However, we already know from #222 that
if the new rep is a signed integer, and the old rep is unsigned, we need
to be careful to use a signed type for intermediate computations.

To preserve this behaviour but restore correct behavoiur for other cases
(namely, #233), we add a new type trait for this intermediate type.  It
basically boils down to the common type, but with an explicit carve-out
to keep #222 fixed.  We explicitly cast both participants to this new
type-for-intermediate-computations, and then cast to the destination rep
at the end.

Compile time tests showed no discernible impact at all.  This change has
also passed all Aurora-internal builds.

Fixes #233.
---
 au/quantity_point.hh      | 30 +++++++++++++++++++++++++++++-
 au/quantity_point_test.cc | 11 +++++++++++
 au/quantity_test.cc       |  6 ++++++
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/au/quantity_point.hh b/au/quantity_point.hh
index 816d7b50..db4698f4 100644
--- a/au/quantity_point.hh
+++ b/au/quantity_point.hh
@@ -52,6 +52,9 @@ struct AreQuantityPointTypesEquivalent;
 namespace detail {
 template <typename TargetRep, typename U1, typename U2>
 struct OriginDisplacementFitsIn;
+
+template <typename FromRep, typename ToRep>
+struct IntermediateRep;
 }  // namespace detail
 
 // QuantityPoint implementation and API elaboration.
@@ -132,7 +135,9 @@ class QuantityPoint {
               typename NewUnit,
               typename = std::enable_if_t<IsUnit<NewUnit>::value>>
     constexpr NewRep in(NewUnit u) const {
-        return (rep_cast<NewRep>(x_) - rep_cast<NewRep>(OriginDisplacement<Unit, NewUnit>::value()))
+        using CalcRep = typename detail::IntermediateRep<Rep, NewRep>::type;
+        return (rep_cast<CalcRep>(x_) -
+                rep_cast<CalcRep>(OriginDisplacement<Unit, NewUnit>::value()))
             .template in<NewRep>(u);
     }
 
@@ -435,5 +440,28 @@ struct OriginDisplacementFitsIn
                          stdx::bool_constant<underlying_value_in_range<TargetRep>(
                              OriginDisplacement<U1, U2>::value())>,
                          std::true_type> {};
+
+// We simply want a version of `std::make_signed_t` that won't choke on non-integral types.
+template <typename T, bool IsInt = std::is_integral<T>::value>
+struct MakeSigned;
+template <typename T>
+struct MakeSigned<T, false> : stdx::type_identity<T> {};
+template <typename T>
+struct MakeSigned<T, true> : stdx::type_identity<std::make_signed_t<T>> {};
+
+// If the destination is a signed integer, we want to ensure we do our
+// computations in a signed type.  Otherwise, just use the common type for our
+// intermediate computations.
+template <typename CommonT, bool IsDestinationSigned>
+struct IntermediateRepImpl
+    : std::conditional_t<stdx::conjunction<std::is_integral<CommonT>,
+                                           stdx::bool_constant<IsDestinationSigned>>::value,
+                         MakeSigned<CommonT>,
+                         stdx::type_identity<CommonT>> {};
+
+template <typename FromRep, typename ToRep>
+struct IntermediateRep
+    : IntermediateRepImpl<std::common_type_t<FromRep, ToRep>, std::is_signed<ToRep>::value> {};
+
 }  // namespace detail
 }  // namespace au
diff --git a/au/quantity_point_test.cc b/au/quantity_point_test.cc
index cc7457db..9f911956 100644
--- a/au/quantity_point_test.cc
+++ b/au/quantity_point_test.cc
@@ -195,6 +195,11 @@ TEST(QuantityPoint, CanCastToUnitWithDifferentOrigin) {
     EXPECT_THAT(celsius_pt(10).coerce_as(Kelvins{}), SameTypeAndValue(kelvins_pt(283)));
 }
 
+TEST(QuantityPoint, HandlesConversionWithSignedSourceAndUnsignedDestination) {
+    EXPECT_THAT(celsius_pt(int16_t{-5}).coerce_as<uint16_t>(kelvins_pt),
+                SameTypeAndValue(kelvins_pt(uint16_t{268})));
+}
+
 TEST(QuantityPoint, CoerceAsWillForceLossyConversion) {
     // Truncation.
     EXPECT_THAT(inches_pt(30).coerce_as(feet_pt), SameTypeAndValue(feet_pt(2)));
@@ -240,6 +245,12 @@ TEST(QuantityPoint, CoerceInExplicitRepSetsOutputType) {
     EXPECT_THAT(feet_pt(30).coerce_in<uint8_t>(inches_pt), SameTypeAndValue(uint8_t{104}));
 }
 
+TEST(QuantityPoint, CoerceAsPerformsConversionInWidestType) {
+    constexpr QuantityPointU32<Milli<Kelvins>> temp = milli(kelvins_pt)(313'150u);
+    EXPECT_THAT(temp.coerce_as<uint16_t>(deci(kelvins_pt)),
+                SameTypeAndValue(deci(kelvins_pt)(uint16_t{3131})));
+}
+
 TEST(QuantityPoint, ComparisonsWorkAsExpected) {
     constexpr auto x = meters_pt(3);
 
diff --git a/au/quantity_test.cc b/au/quantity_test.cc
index 733fa619..ed4c4c85 100644
--- a/au/quantity_test.cc
+++ b/au/quantity_test.cc
@@ -253,6 +253,12 @@ TEST(Quantity, CoerceInExplicitRepSetsOutputType) {
     EXPECT_THAT(feet(30).coerce_in<uint8_t>(inches), SameTypeAndValue(uint8_t{104}));
 }
 
+TEST(Quantity, CoerceAsPerformsConversionInWidestType) {
+    constexpr QuantityU32<Milli<Meters>> length = milli(meters)(313'150u);
+    EXPECT_THAT(length.coerce_as<uint16_t>(deci(meters)),
+                SameTypeAndValue(deci(meters)(uint16_t{3131})));
+}
+
 TEST(Quantity, CanImplicitlyConvertToDifferentUnitOfSameDimension) {
     constexpr QuantityI32<Inches> x = yards(2);
     EXPECT_EQ(x.in(inches), 72);