Skip to content

Commit

Permalink
Deprecate bare-offset-unit maker names (#35)
Browse files Browse the repository at this point in the history
If a unit has a nonzero offset, then that implies it will typically be
used as a _point_, rather than a _quantity_.  So far, our convention has
been that quantity makers get the name of the unit, and quantity _point_
makers get that name with a `_pt` suffix.  However, this turns out to be
really error prone for offset units: the default name will give users
something that behaves in surprising ways.

As a more concrete example, `celsius(0)` is equal to `fahrenheit(0)`
currently.  Since these are quantities, this makes sense!  But to the
reader, these just "look like temperatures" (which are _points_), so
the appearance is misleading and will lead to mistakes.

We _could_ say that the "bare unit name" for _offset_ units belongs to
the quantity _point_ maker.  But then it becomes harder to reason about
what "kind of thing" a bare-named maker will return.  It also creates an
unfortunate inconsistency between `celsius` and `kelvins`, say.

I think the better answer is to recognize that a name like `celsius` is
_intrinsically_ ambiguous.  Since it's the one users will reach for, we
can keep it in the library, but purely for the purpose of giving a
readable deprecation warning.  We will tell users to say `celsius_pt` if
that's what they mean, and `celsius_qty` (using a new `_qty` suffix
convention) otherwise.

Fixes #34.
  • Loading branch information
chiphogg authored Dec 20, 2022
1 parent db68f2f commit 664cf7e
Show file tree
Hide file tree
Showing 7 changed files with 19 additions and 11 deletions.
4 changes: 2 additions & 2 deletions au/io_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ struct Celsius : Kelvins {
static constexpr auto origin() { return centi(kelvins)(273'15); }
};
constexpr const char Celsius::label[];
constexpr auto celsius = QuantityMaker<Celsius>{};
constexpr auto celsius_qty = QuantityMaker<Celsius>{};
constexpr auto celsius_pt = QuantityPointMaker<Celsius>{};

} // namespace
Expand All @@ -50,7 +50,7 @@ TEST(StreamingOutput, PrintsValueAndUnitLabel) {
}

TEST(StreamingOutput, DistinguishesPointFromQuantityByAtSign) {
EXPECT_EQ(stream_to_string(celsius(20)), "20 deg C");
EXPECT_EQ(stream_to_string(celsius_qty(20)), "20 deg C");
EXPECT_EQ(stream_to_string(celsius_pt(20)), "@(20 deg C)");

EXPECT_EQ(stream_to_string(kelvins(20)), "20 K");
Expand Down
8 changes: 4 additions & 4 deletions au/quantity_point_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ constexpr QuantityPointMaker<Kelvins> kelvins_pt{};
struct Celsius : Kelvins {
static constexpr auto origin() { return centi(kelvins)(273'15); }
};
constexpr QuantityMaker<Celsius> celsius{};
constexpr QuantityMaker<Celsius> celsius_qty{};
constexpr QuantityPointMaker<Celsius> celsius_pt{};

TEST(QuantityPoint, HasExpectedDiffType) {
Expand Down Expand Up @@ -216,7 +216,7 @@ TEST(QuantityPoint, MixedUnitAdditionUsesCommonDenominator) {

TEST(QuantityPoint, MixedUnitAdditionWithQuantityDoesNotConsiderOrigin) {
EXPECT_THAT(celsius_pt(20) + kelvins(5), PointEquivalent(celsius_pt(25)));
EXPECT_THAT(celsius(20) + kelvins_pt(5), PointEquivalent(kelvins_pt(25)));
EXPECT_THAT(celsius_qty(20) + kelvins_pt(5), PointEquivalent(kelvins_pt(25)));
}

TEST(QuantityPoint, MixedUnitSubtractionUsesCommonDenominator) {
Expand All @@ -229,7 +229,7 @@ TEST(QuantityPoint, MixedUnitSubtractionWithQuantityDoesNotConsiderOrigin) {
}

TEST(QuantityPoint, MixedUnitsWithIdenticalNonzeroOriginDontGetSubdivided) {
constexpr auto diff = kilo(celsius_pt)(1) - celsius(900);
constexpr auto diff = kilo(celsius_pt)(1) - celsius_qty(900);
EXPECT_THAT(diff, PointEquivalent(celsius_pt(100)));

// Just to leave no doubt: the centi-celsius units of the origin should _not_ influence the
Expand Down Expand Up @@ -279,7 +279,7 @@ TEST(QuantityPoint, InheritsOverflowSafetySurfaceFromUnderlyingQuantityTypes) {
// Note that this is explicitly due to the influence of the origin _difference_. For
// _quantities_, rather than quantity _points_, this would work just fine, as the following test
// shows.
ASSERT_TRUE(celsius(static_cast<uint16_t>(20)) < kelvins(static_cast<uint16_t>(293)));
ASSERT_TRUE(celsius_qty(static_cast<uint16_t>(20)) < kelvins(static_cast<uint16_t>(293)));
}

TEST(QuantityPointMaker, CanApplyPrefix) {
Expand Down
6 changes: 5 additions & 1 deletion au/units/celsius.hh
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ struct Celsius : Kelvins, CelsiusLabel<void> {
using CelsiusLabel<void>::label;
static constexpr auto origin() { return centi(kelvins)(273'15); }
};
constexpr auto celsius = QuantityMaker<Celsius>{};
constexpr auto celsius_qty = QuantityMaker<Celsius>{};
constexpr auto celsius_pt = QuantityPointMaker<Celsius>{};

[[deprecated(
"`celsius()` is ambiguous. Use `celsius_pt()` for _points_, or `celsius_qty()` for "
"_quantities_")]] constexpr auto celsius = QuantityMaker<Celsius>{};

} // namespace au
6 changes: 5 additions & 1 deletion au/units/fahrenheit.hh
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ struct Fahrenheit : Rankines, FahrenheitLabel<void> {
using FahrenheitLabel<void>::label;
static constexpr auto origin() { return centi(rankines)(459'67); }
};
constexpr auto fahrenheit = QuantityMaker<Fahrenheit>{};
constexpr auto fahrenheit_qty = QuantityMaker<Fahrenheit>{};
constexpr auto fahrenheit_pt = QuantityPointMaker<Fahrenheit>{};

[[deprecated(
"`fahrenheit()` is ambiguous. Use `fahrenheit_pt()` for _points_, or `fahrenheit_qty()` for "
"_quantities_")]] constexpr auto fahrenheit = QuantityMaker<Fahrenheit>{};

} // namespace au
2 changes: 1 addition & 1 deletion au/units/test/celsius_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace au {
TEST(Celsius, HasExpectedLabel) { expect_label<Celsius>("degC"); }

TEST(Celsius, QuantityEquivalentToKelvins) {
EXPECT_THAT(celsius(20), QuantityEquivalent(kelvins(20)));
EXPECT_THAT(celsius_qty(20), QuantityEquivalent(kelvins(20)));
}

TEST(Celsius, QuantityPointHasCorrectOffsetFromKelvins) {
Expand Down
2 changes: 1 addition & 1 deletion au/units/test/fahrenheit_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace au {
TEST(Fahrenheit, HasExpectedLabel) { expect_label<Fahrenheit>("F"); }

TEST(Fahrenheit, HasCorrectQuantityRelationshipWithCelsius) {
EXPECT_EQ(fahrenheit(9), celsius(5));
EXPECT_EQ(fahrenheit_qty(9), celsius_qty(5));
}

TEST(Fahrenheit, HasCorrectRelationshipsWithCelsius) {
Expand Down
2 changes: 1 addition & 1 deletion au/units/test/kelvins_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace au {
TEST(Kelvins, HasExpectedLabel) { expect_label<Kelvins>("K"); }

TEST(Kelvins, QuantityEquivalentToCelsius) {
EXPECT_THAT(kelvins(10), QuantityEquivalent(celsius(10)));
EXPECT_THAT(kelvins(10), QuantityEquivalent(celsius_qty(10)));
}

} // namespace au

0 comments on commit 664cf7e

Please sign in to comment.