Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate redundant units from CommonUnit<...> #310

Merged
merged 1 commit into from
Oct 28, 2024
+149 −5
Merged

Eliminate redundant units from CommonUnit<...> #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion au/code/au/quantity_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,19 @@
#include "gtest/gtest.h"

using ::testing::DoubleEq;
using ::testing::Each;
using ::testing::Eq;
using ::testing::StaticAssertTypeEq;

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>{};

Expand Down Expand Up @@ -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));
}
Comment on lines +725 to +749
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewer: if we added only this test case on main, without the fixes, here is what the failure would look like:

[ RUN      ] Quantity.CommonUnitAlwaysCompletelyIndependentOfOrder
au/code/au/quantity_test.cc:743: Failure
Value of: results
Expected: only contains elements that is equal to "805177 COM[cm, m, mi]"
  Actual: { "805177 COM[cm, m, mi]", "805177 COM[cm, mi]", "805177 COM[cm, m, mi]", "805177 COM[cm, m, mi]", "805177 COM[cm, mi]", "805177 COM[cm, m, mi]" }, whose element #1 doesn't match
Inconsistency found for (1 cm, 1 mi, 1 m)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking that they are all consistent with the first element, can we check that they are all equal to a specific unit? That might help test readibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the appeal of this suggestion. I struggled with this for a little bit, but I couldn't see my way to an implementation that did this while keeping the test implementation clean.

One way would be to compare to a desired unit label. But the unit label for common units will change dramatically after 0.4.0 (#105), and I'd hate to have to update this test for that change.

We could also do it by providing the expected type of the common unit. But CommonUnit<...> isn't supposed to be named directly by end users: we're supposed to use CommonUnitT<...>, which performs the de-duping and simplification that is the very thing under test!

Ultimately, I think the current approach is the best option available. Phrasing it in terms of order independence, as mentioned in #295 (comment), is the most elegant way I know to express what we're really after here. The downside, as you point out, is that a test failure can't tell you which one is correct; all it can do is point out that they should all be the same, and they're not. But I think in practice this will be good enough to point maintainers in the right direction.


TEST(Quantity, CommonTypeRespectsImplicitRepSafetyChecks) {
// The following test should fail to compile. Uncomment both lines to check.
// constexpr auto feeters = QuantityMaker<CommonUnitT<Meters, Feet>>{};
Expand Down
52 changes: 51 additions & 1 deletion au/code/au/unit_of_measure.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
26 changes: 26 additions & 0 deletions au/code/au/unit_of_measure_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, "") {
Expand Down Expand Up @@ -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
20 changes: 17 additions & 3 deletions au/code/au/utility/test/type_traits_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
23 changes: 23 additions & 0 deletions au/code/au/utility/type_traits.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>> {};

Expand All @@ -39,11 +44,29 @@ struct AlwaysFalse : std::false_type {};

////////////////////////////////////////////////////////////////////////////////////////////////////
// Implementation details below.
////////////////////////////////////////////////////////////////////////////////////////////////////

////////////////////////////////////////////////////////////////////////////////////////////////////
// `Prepend` implementation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of this comment? Do we need a comment for all inline implementations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Often, the implementation for "public"-ish interface declared up top in a file will depend on new helpers, which I will declare-and-define right next to the "core" implementation. This can get a little messy and confusing, so I use these "section comments" to group things and clarify the purpose. That said, I'm certainly rather inconsistent about using them in practice.


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
Loading