From 5f79db31b530391c1d27c50532745f52fe5b1e7f Mon Sep 17 00:00:00 2001
From: Chip Hogg <chogg@aurora.tech>
Date: Mon, 28 Oct 2024 11:14:32 -0400
Subject: [PATCH] Eliminate redundant units from `CommonUnit<...>` (#310)

The core here is the addition to `:quantity_test`.  Making this pass is
the goal of this PR.

To get there, we need a new trait/metafunction that can take a pack of
units, and eliminate all the "redundant" ones.  This means we also need
a definition of "redundant", where the canonical example would be that
"feet" is redundant between "feet" and "inches".  Here's what we came up
with:

- If two units are _identical_ (same type), then each is redundant
  w.r.t. the other.
- If two units are _distinct_, but _quantity-equivalent_, then the
  redundant one is whichever comes later in the "standard pack
  ordering".  (We try to give more "recognizable" units higher
  priority.)
- Otherwise, for _quantity-inequivalent_ units, a redundant unit is an
  exact integer multiple of another unit.

All of this assumes that the units have the same dimension.  We simply
wouldn't call this machinery in the first place if that weren't true.

Finally, we need one more piece of machinery: a simple "drop all X"
metafunction.  This is because when an earlier unit makes later units
redundant, we can't easily "reach into" that "tail pack" of units and
remove them directly.  Instead, we need to mark them with a "tombstone"
(`void`), and then remove all the tombstoned units.

Fixes #295.
---
 au/code/au/quantity_test.cc                 | 33 ++++++++++++-
 au/code/au/unit_of_measure.hh               | 52 ++++++++++++++++++++-
 au/code/au/unit_of_measure_test.cc          | 26 +++++++++++
 au/code/au/utility/test/type_traits_test.cc | 20 ++++++--
 au/code/au/utility/type_traits.hh           | 23 +++++++++
 5 files changed, 149 insertions(+), 5 deletions(-)

diff --git a/au/code/au/quantity_test.cc b/au/code/au/quantity_test.cc
index e5f1d4e0..39d6d8ff 100644
--- a/au/code/au/quantity_test.cc
+++ b/au/code/au/quantity_test.cc
@@ -22,6 +22,8 @@
 #include "gtest/gtest.h"
 
 using ::testing::DoubleEq;
+using ::testing::Each;
+using ::testing::Eq;
 using ::testing::StaticAssertTypeEq;
 
 namespace au {
@@ -29,7 +31,10 @@ namespace au {
 struct Feet : UnitImpl<Length> {};
 constexpr auto feet = QuantityMaker<Feet>{};
 
-struct Miles : decltype(Feet{} * mag<5'280>()) {};
+struct Miles : decltype(Feet{} * mag<5'280>()) {
+    static constexpr const char label[] = "mi";
+};
+constexpr const char Miles::label[];
 constexpr auto mile = SingularNameFor<Miles>{};
 constexpr auto miles = QuantityMaker<Miles>{};
 
@@ -717,6 +722,32 @@ TEST(Quantity, MixedTypeSubtractionUsesCommonRepType) {
     EXPECT_THAT(feet(2.f) - feet(1.5), QuantityEquivalent(feet(0.5)));
 }
 
+TEST(Quantity, CommonUnitAlwaysCompletelyIndependentOfOrder) {
+    auto check_units = [](auto unit_a, auto unit_b, auto unit_c) {
+        const auto a = unit_a(1LL);
+        const auto b = unit_b(1LL);
+        const auto c = unit_c(1LL);
+        auto stream_to_string = [](auto x) {
+            std::ostringstream oss;
+            oss << x;
+            return oss.str();
+        };
+        std::vector<std::string> results = {
+            stream_to_string(a + b + c),
+            stream_to_string(a + c + b),
+            stream_to_string(b + a + c),
+            stream_to_string(b + c + a),
+            stream_to_string(c + a + b),
+            stream_to_string(c + b + a),
+        };
+        EXPECT_THAT(results, Each(Eq(results[0])))
+            << "Inconsistency found for (" << a << ", " << b << ", " << c << ")";
+    };
+
+    check_units(centi(meters), miles, meters);
+    check_units(kilo(meters), miles, milli(meters));
+}
+
 TEST(Quantity, CommonTypeRespectsImplicitRepSafetyChecks) {
     // The following test should fail to compile.  Uncomment both lines to check.
     // constexpr auto feeters = QuantityMaker<CommonUnitT<Meters, Feet>>{};
diff --git a/au/code/au/unit_of_measure.hh b/au/code/au/unit_of_measure.hh
index 20b2c8de..d88f335d 100644
--- a/au/code/au/unit_of_measure.hh
+++ b/au/code/au/unit_of_measure.hh
@@ -19,6 +19,7 @@
 #include "au/power_aliases.hh"
 #include "au/stdx/type_traits.hh"
 #include "au/utility/string_constant.hh"
+#include "au/utility/type_traits.hh"
 #include "au/zero.hh"
 
 namespace au {
@@ -533,10 +534,59 @@ struct FirstMatchingUnit<Matcher, TargetUnit, List<H, Ts...>>
                          stdx::type_identity<H>,
                          FirstMatchingUnit<Matcher, TargetUnit, List<Ts...>>> {};
 
+// A "redundant" unit, among a list of units, is one that is an exact integer multiple of another.
+//
+// If two units are identical, then each is redundant with the other.
+//
+// If two units are distinct, but quantity-equivalent, then the unit that comes later in the
+// standard unit ordering (i.e., `InOrderFor<Pack, ...>`) is the redundant one.
+template <typename Pack>
+struct EliminateRedundantUnitsImpl;
+template <typename Pack>
+using EliminateRedundantUnits = typename EliminateRedundantUnitsImpl<Pack>::type;
+
+// Base case: no units to eliminate.
+template <template <class...> class Pack>
+struct EliminateRedundantUnitsImpl<Pack<>> : stdx::type_identity<Pack<>> {};
+
+// Helper for recursive case.
+template <template <class...> class Pack, typename U1, typename U2>
+struct IsFirstUnitRedundant
+    : std::conditional_t<std::is_same<U1, U2>::value,
+                         std::true_type,
+                         std::conditional_t<AreUnitsQuantityEquivalent<U1, U2>::value,
+                                            InOrderFor<Pack, U2, U1>,
+                                            IsInteger<UnitRatioT<U1, U2>>>> {};
+
+// Recursive case: eliminate first unit if it is redundant; else, keep it and eliminate any later
+// units that are redundant with it.
+template <template <class...> class Pack, typename H, typename... Ts>
+struct EliminateRedundantUnitsImpl<Pack<H, Ts...>>
+    : std::conditional<
+
+          // If `H` is redundant with _any later unit_, simply omit it.
+          stdx::disjunction<IsFirstUnitRedundant<Pack, H, Ts>...>::value,
+          EliminateRedundantUnits<Pack<Ts...>>,
+
+          // Otherwise, we know we'll need to keep `H`, so we prepend it to the remaining result.
+          //
+          // To get that result, we first replace any units _that `H` makes redundant_ with `void`.
+          // Then, we drop all `void`, before finally recursively eliminating any units that are
+          // redundant among those that remain.
+          PrependT<
+              EliminateRedundantUnits<DropAll<
+                  void,
+
+                  // `Pack<Ts...>`, but with redundant-with-`H` units replaced by `void`:
+                  Pack<std::conditional_t<IsFirstUnitRedundant<Pack, Ts, H>::value, void, Ts>...>>>,
+
+              H>> {};
+
 }  // namespace detail
 
 template <typename... Us>
-using ComputeCommonUnitImpl = FlatDedupedTypeListT<CommonUnit, Us...>;
+using ComputeCommonUnitImpl =
+    detail::EliminateRedundantUnits<FlatDedupedTypeListT<CommonUnit, Us...>>;
 
 template <typename... Us>
 struct ComputeCommonUnit
diff --git a/au/code/au/unit_of_measure_test.cc b/au/code/au/unit_of_measure_test.cc
index e8430101..dfd452cf 100644
--- a/au/code/au/unit_of_measure_test.cc
+++ b/au/code/au/unit_of_measure_test.cc
@@ -83,6 +83,12 @@ struct SomeUnitWrapper {};
 template <typename UnitT>
 struct AssociatedUnit<SomeUnitWrapper<UnitT>> : stdx::type_identity<UnitT> {};
 
+// Useful for testing parameter pack logic.
+template <typename... Units>
+struct SomePack {};
+template <typename A, typename B>
+struct InOrderFor<SomePack, A, B> : InOrderFor<CommonUnit, A, B> {};
+
 struct UnlabeledUnit : decltype(Feet{} * mag<9>()) {};
 
 MATCHER_P(QuantityEquivalentToUnit, target, "") {
@@ -637,5 +643,25 @@ TEST(CommonOrigin, SymmetricUnderReordering) {
     EXPECT_THAT(common_origin_value, Not(SameTypeAndValue(AlternateCelsius::origin())));
 }
 
+TEST(EliminateRedundantUnits, IdentityForEmptySet) {
+    StaticAssertTypeEq<EliminateRedundantUnits<SomePack<>>, SomePack<>>();
+}
+
+TEST(EliminateRedundantUnits, IdentityForSingleUnit) {
+    StaticAssertTypeEq<EliminateRedundantUnits<SomePack<Feet>>, SomePack<Feet>>();
+}
+
+TEST(EliminateRedundantUnits, RemovesRepeatedUnit) {
+    StaticAssertTypeEq<EliminateRedundantUnits<SomePack<Feet, Feet>>, SomePack<Feet>>();
+    StaticAssertTypeEq<EliminateRedundantUnits<SomePack<Feet, Meters, Feet>>,
+                       SomePack<Meters, Feet>>();
+}
+
+TEST(EliminateRedundantUnits, AlwaysRemovesSameUnitAmongQuantityEquivalentChoices) {
+    using Twelvinch = decltype(Inches{} * mag<12>());
+    StaticAssertTypeEq<EliminateRedundantUnits<SomePack<Feet, Twelvinch>>,
+                       EliminateRedundantUnits<SomePack<Twelvinch, Feet>>>();
+}
+
 }  // namespace detail
 }  // namespace au
diff --git a/au/code/au/utility/test/type_traits_test.cc b/au/code/au/utility/test/type_traits_test.cc
index 821bca40..3732fca4 100644
--- a/au/code/au/utility/test/type_traits_test.cc
+++ b/au/code/au/utility/test/type_traits_test.cc
@@ -22,11 +22,11 @@ namespace au {
 namespace detail {
 
 template <typename... Ts>
-struct TestingPack;
+struct Pack;
 
 TEST(Prepend, PrependsToPack) {
-    StaticAssertTypeEq<PrependT<TestingPack<>, int>, TestingPack<int>>();
-    StaticAssertTypeEq<PrependT<TestingPack<double, char>, int>, TestingPack<int, double, char>>();
+    StaticAssertTypeEq<PrependT<Pack<>, int>, Pack<int>>();
+    StaticAssertTypeEq<PrependT<Pack<double, char>, int>, Pack<int, double, char>>();
 }
 
 TEST(SameTypeIgnoringCvref, IgnoresCvrefQualifiers) {
@@ -51,5 +51,19 @@ TEST(AlwaysFalse, IsAlwaysFalse) {
     EXPECT_FALSE((AlwaysFalse<int, char, double>::value));
 }
 
+TEST(DropAll, IdentityWhenTargetAbsent) {
+    StaticAssertTypeEq<DropAll<void, Pack<>>, Pack<>>();
+    StaticAssertTypeEq<DropAll<void, Pack<int>>, Pack<int>>();
+    StaticAssertTypeEq<DropAll<void, Pack<int, char, double>>, Pack<int, char, double>>();
+}
+
+TEST(DropAll, DropsAllInstancesOfTarget) {
+    StaticAssertTypeEq<DropAll<int, Pack<int>>, Pack<>>();
+    StaticAssertTypeEq<DropAll<int, Pack<int, int>>, Pack<>>();
+    StaticAssertTypeEq<DropAll<int, Pack<int, char, int>>, Pack<char>>();
+    StaticAssertTypeEq<DropAll<int, Pack<char, int, char>>, Pack<char, char>>();
+    StaticAssertTypeEq<DropAll<int, Pack<int, char, int, double>>, Pack<char, double>>();
+}
+
 }  // namespace detail
 }  // namespace au
diff --git a/au/code/au/utility/type_traits.hh b/au/code/au/utility/type_traits.hh
index ce276788..d4c5a615 100644
--- a/au/code/au/utility/type_traits.hh
+++ b/au/code/au/utility/type_traits.hh
@@ -26,6 +26,11 @@ struct Prepend;
 template <typename PackT, typename T>
 using PrependT = typename Prepend<PackT, T>::type;
 
+template <typename T, typename Pack>
+struct DropAllImpl;
+template <typename T, typename Pack>
+using DropAll = typename DropAllImpl<T, Pack>::type;
+
 template <typename T, typename U>
 struct SameTypeIgnoringCvref : std::is_same<stdx::remove_cvref_t<T>, stdx::remove_cvref_t<U>> {};
 
@@ -39,11 +44,29 @@ struct AlwaysFalse : std::false_type {};
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////
 // Implementation details below.
+////////////////////////////////////////////////////////////////////////////////////////////////////
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// `Prepend` implementation.
 
 template <template <typename...> class Pack, typename T, typename... Us>
 struct Prepend<Pack<Us...>, T> {
     using type = Pack<T, Us...>;
 };
 
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// `DropAll` implementation.
+
+// Base case.
+template <typename T, template <class...> class Pack>
+struct DropAllImpl<T, Pack<>> : stdx::type_identity<Pack<>> {};
+
+// Recursive case:
+template <typename T, template <class...> class Pack, typename H, typename... Ts>
+struct DropAllImpl<T, Pack<H, Ts...>>
+    : std::conditional<std::is_same<T, H>::value,
+                       DropAll<T, Pack<Ts...>>,
+                       detail::PrependT<DropAll<T, Pack<Ts...>>, H>> {};
+
 }  // namespace detail
 }  // namespace au