From 50de6e9fc7894279aa8d27c6b9643cfedfe3b6cc Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Mon, 24 Jun 2024 10:45:24 -0400 Subject: [PATCH] Deprecate `PI` (#250) If we have anything called `PI` --- even if it is properly encapsulated inside a C++ namespace! --- then users will be unable to use our library if they also depend on _any library_ that defines a macro called `PI`, in _any_ namespace (because macros _do not care_ about namespaces). Of course, defining a macro named `PI` would be an execrable and amateurish violation of long-accepted best practices for C++ software engineering. However, at least one library does this: the Arduino core library. Therefore, if we want to support it --- which we most certainly do --- then we will have to yield. The plan going forward is to release this deprecation as part of 0.3.5, and then fully delete the variable as part of 0.4.0. In the meantime, users can still get Au to work; they just need to make sure that they include Au _after_ any files that --- _lamentably_ --- define a macro `PI`. We have other "short all-caps" identifiers, `ONE` and `ZERO`. We could get rid of `ONE` with no trouble at all; `mag<1>()` is a fine replacement. Getting rid of `ZERO` would be extremely painful, though. The motivation to define a `ZERO` macro is far less than for `PI`, so I am planning to simply hope that no library of note has done this, and expecting to tell users of any such library that they are simply out of luck. Fixes #246. --- au/apply_magnitude_test.cc | 2 ++ au/constant_test.cc | 4 +++- au/conversion_policy_test.cc | 4 ++++ au/magnitude.hh | 4 +++- au/magnitude_test.cc | 4 +++- au/rep_test.cc | 2 +- au/units/test/degrees_test.cc | 2 +- au/units/test/radians_test.cc | 2 +- au/wrapper_operations_test.cc | 2 ++ docs/discussion/implementation/vector_space.md | 4 ++-- docs/howto/new-units.md | 2 +- docs/install.md | 6 +++--- docs/reference/corresponding_quantity.md | 2 +- docs/reference/detail/monovalue_types.md | 1 - docs/reference/magnitude.md | 17 +++++++++-------- release/common_test_cases.hh | 3 +++ 16 files changed, 39 insertions(+), 22 deletions(-) diff --git a/au/apply_magnitude_test.cc b/au/apply_magnitude_test.cc index 1f6fad24..b6aab045 100644 --- a/au/apply_magnitude_test.cc +++ b/au/apply_magnitude_test.cc @@ -23,6 +23,8 @@ using ::testing::Not; namespace au { namespace detail { namespace { +constexpr auto PI = Magnitude{}; + template std::vector first_n_positive_values(std::size_t n) { std::vector result; diff --git a/au/constant_test.cc b/au/constant_test.cc index 43b3f2a3..4cd10460 100644 --- a/au/constant_test.cc +++ b/au/constant_test.cc @@ -33,6 +33,8 @@ using ::testing::StrEq; namespace au { namespace { +constexpr auto PI = Magnitude{}; + template std::string stream_to_string(Quantity q) { std::ostringstream oss; @@ -60,7 +62,6 @@ struct PlancksConstant : decltype(Joules{} * Seconds{} * H_JS) { }; constexpr auto plancks_constant = QuantityMaker{}; constexpr auto h = make_constant(plancks_constant); -} // namespace TEST(MakeConstant, MakesConstantFromUnit) { StaticAssertTypeEq>(); @@ -282,4 +283,5 @@ TEST(CanStoreValueIn, ChecksRangeOfTypeForIntegers) { EXPECT_FALSE(decltype(c)::can_store_value_in(meters / second)); } +} // namespace } // namespace au diff --git a/au/conversion_policy_test.cc b/au/conversion_policy_test.cc index 531014c7..b1dde31e 100644 --- a/au/conversion_policy_test.cc +++ b/au/conversion_policy_test.cc @@ -18,6 +18,9 @@ #include "gtest/gtest.h" namespace au { +namespace { + +constexpr auto PI = Magnitude{}; struct Grams : UnitImpl {}; struct Kilograms : decltype(Grams{} * pow<3>(mag<10>())) {}; @@ -138,4 +141,5 @@ TEST(ConstructionPolicy, OkForIntegralRepAndEquivalentUnit) { (ConstructionPolicy::PermitImplicitFrom::value)); } +} // namespace } // namespace au diff --git a/au/magnitude.hh b/au/magnitude.hh index 26fafb50..1be7957e 100644 --- a/au/magnitude.hh +++ b/au/magnitude.hh @@ -139,7 +139,9 @@ static constexpr auto ONE = Magnitude<>{}; // // If you are stuck with such a framework, you can choose a different name that does not collide, // and reproduce the following line in your own system. -static constexpr auto PI = Magnitude{}; +[[deprecated( + "If you need a magnitude instance for pi, define your own as `constexpr auto PI = " + "Magnitude{};`")]] static constexpr auto PI = Magnitude{}; #endif template diff --git a/au/magnitude_test.cc b/au/magnitude_test.cc index 1f9005fb..ccf818ff 100644 --- a/au/magnitude_test.cc +++ b/au/magnitude_test.cc @@ -26,11 +26,12 @@ using ::testing::StaticAssertTypeEq; namespace au { namespace { +constexpr auto PI = Magnitude{}; + template ::value>> constexpr T cubed(T x) { return x * x * x; } -} // namespace TEST(Magnitude, SupportsEqualityComparison) { constexpr auto mag_1 = mag<1>(); @@ -270,6 +271,7 @@ TEST(CommonMagnitude, ZeroResultIndicatesAllInputsAreZero) { EXPECT_EQ(common_magnitude(ZERO, ZERO, ZERO), ZERO); EXPECT_EQ(common_magnitude(ZERO, ZERO, ZERO, ZERO, ZERO), ZERO); } +} // namespace namespace detail { diff --git a/au/rep_test.cc b/au/rep_test.cc index 8afe306f..a2409973 100644 --- a/au/rep_test.cc +++ b/au/rep_test.cc @@ -78,7 +78,7 @@ TEST(IsValidRep, TrueForStdComplex) { TEST(IsValidRep, FalseForMagnitude) { EXPECT_FALSE(IsValidRep())>::value); - EXPECT_FALSE(IsValidRep::value); + EXPECT_FALSE(IsValidRep{}))>::value); } TEST(IsValidRep, FalseForUnits) { diff --git a/au/units/test/degrees_test.cc b/au/units/test/degrees_test.cc index 25480285..8914100b 100644 --- a/au/units/test/degrees_test.cc +++ b/au/units/test/degrees_test.cc @@ -23,7 +23,7 @@ namespace au { TEST(Degrees, HasExpectedLabel) { expect_label("deg"); } TEST(Degrees, RoughlyEquivalentToPiOver180Radians) { - EXPECT_DOUBLE_EQ(degrees(1.0).in(radians), get_value(PI / mag<180>())); + EXPECT_DOUBLE_EQ(degrees(1.0).in(radians), get_value(Magnitude{} / mag<180>())); } TEST(Degrees, One360thOfARevolution) { EXPECT_EQ(degrees(360), revolutions(1)); } diff --git a/au/units/test/radians_test.cc b/au/units/test/radians_test.cc index 1980f56d..35b02eb8 100644 --- a/au/units/test/radians_test.cc +++ b/au/units/test/radians_test.cc @@ -23,7 +23,7 @@ namespace au { TEST(Radians, HasExpectedLabel) { expect_label("rad"); } TEST(Radians, TwoPiPerRevolution) { - EXPECT_DOUBLE_EQ(radians(get_value(mag<2>() * PI)).in(revolutions), 1.0); + EXPECT_DOUBLE_EQ(radians(get_value(mag<2>() * Magnitude{})).in(revolutions), 1.0); } TEST(Radians, HasExpectedSymbol) { diff --git a/au/wrapper_operations_test.cc b/au/wrapper_operations_test.cc index cac07e08..a3c0f53d 100644 --- a/au/wrapper_operations_test.cc +++ b/au/wrapper_operations_test.cc @@ -26,6 +26,8 @@ namespace au { namespace detail { namespace { +constexpr auto PI = Magnitude{}; + template struct UnitWrapper : MakesQuantityFromNumber, ScalesQuantity, diff --git a/docs/discussion/implementation/vector_space.md b/docs/discussion/implementation/vector_space.md index f657ce76..6be273dd 100644 --- a/docs/discussion/implementation/vector_space.md +++ b/docs/discussion/implementation/vector_space.md @@ -147,9 +147,9 @@ representations, though, its difficulty becomes a strength. We know there is no exponents $\{a_i\}$ such that $\pi = \prod\limits_{i=1}^N p_i^{a_i}$, for any collection of primes $\{p_i\}$. This means that $\pi$ is **independent**, and we can add it as a new basis vector. Then the ratio of, say, `Degrees` to `Radians` (i.e., $\pi / 180$) could be expressed as -`PI / mag<180>()`[^4]. +`Magnitude{} / mag<180>()`[^4]. -[^4]: `PI / mag<180>()` expands to `Magnitude, -2>, Pow, -2>, Pi, +[^4]: `Magnitude{} / mag<180>()` expands to `Magnitude, -2>, Pow, -2>, Pi, Pow, -1>>`. ### Units diff --git a/docs/howto/new-units.md b/docs/howto/new-units.md index 81e4c089..3f087a65 100644 --- a/docs/howto/new-units.md +++ b/docs/howto/new-units.md @@ -147,7 +147,7 @@ Here are some example unit expressions we might reach for to define various comm - Newtons: `Kilo{} * Meters{} / squared(Seconds{})` - Miles: `Feet{} * mag<5280>()` -- Degrees: `Radians{} * PI / mag<180>()` +- Degrees: `Radians{} * Magnitude{} / mag<180>()` ## Aliases vs. strong types: best practices diff --git a/docs/install.md b/docs/install.md index 16087527..840e4821 100644 --- a/docs/install.md +++ b/docs/install.md @@ -107,8 +107,8 @@ Every single-file package automatically includes the following features: - Basic "unit container" types: [`Quantity`](./reference/quantity.md), [`QuantityPoint`](./reference/quantity_point.md) -- [Magnitude](./reference/magnitude.md) types and values, including the constant `PI`, and constants - for any integer such as `mag<5280>()`. +- [Magnitude](./reference/magnitude.md) types and values, including constants for any integer such + as `mag<5280>()`. - All [prefixes](./reference/prefix.md) for SI (`kilo`, `mega`, ...) and informational (`kibi`, `mebi`, ...) quantities. - [Math functions](./reference/math.md), including unit-aware rounding and inverses, trigonometric @@ -154,7 +154,7 @@ should get you any other unit you're likely to want. The units we include are: **scale** a unit by multiplying by Magnitude objects. For example: ```cpp - constexpr auto degrees = radians * PI / mag<180>(); + constexpr auto degrees = radians * Magnitude{} / mag<180>(); ``` These will "work", in the sense of producing correct results. But these ad hoc unit definitions diff --git a/docs/reference/corresponding_quantity.md b/docs/reference/corresponding_quantity.md index 92d1d210..aa631a16 100644 --- a/docs/reference/corresponding_quantity.md +++ b/docs/reference/corresponding_quantity.md @@ -91,7 +91,7 @@ following example. ??? example "Example of 'two-hop' conversion, continued from above" ```cpp - MyDegrees angle = radians(get_value(PI / mag<2>())); + MyDegrees angle = radians(get_value(Magnitude{} / mag<2>())); ``` Here we have a "two-hop" conversion. The corresponding quantity for `MyDegrees` is diff --git a/docs/reference/detail/monovalue_types.md b/docs/reference/detail/monovalue_types.md index a75f5552..89c8fc02 100644 --- a/docs/reference/detail/monovalue_types.md +++ b/docs/reference/detail/monovalue_types.md @@ -36,7 +36,6 @@ Here are some canonical examples in Au. |------|----------|------------| | `Zero` | `ZERO` | Comparing to any `Quantity` | | `Magnitude<>` | `ONE` |
  • Equality comparison with other Magnitudes
  • `get_value(ONE)`
| -| `Magnitude` | `PI` |
  • Equality comparison with other Magnitudes
  • `get_value(PI)`
| | `Radians` (and other units) | `Radians{}` (no special pre-formed instance) | Arithmetic with other units, such as `Radians{} / Meters{}` | ## Switching between types and values {#switching} diff --git a/docs/reference/magnitude.md b/docs/reference/magnitude.md index 8e19f592..168607e9 100644 --- a/docs/reference/magnitude.md +++ b/docs/reference/magnitude.md @@ -20,8 +20,6 @@ There are 3 **valid** ways for end users to form a `Magnitude` instance. 3. :heavy_check_mark: Forming products, quotients, powers, and roots of other valid `Magnitude` instances. -End users can also use pre-formed `Magnitude` instances from the library, such as `PI` and `ONE`. - The following is a **valid, but dis-preferred way** to form a `Magnitude`. - :warning: `Magnitude<>`. @@ -71,10 +69,12 @@ also automatically support related values such as $\pi^2$, $\frac{1}{\sqrt{2\pi} cases, such as handling $\pi$, which most units libraries have traditionally struggled to support. -Because of its importance for angular variables, $\pi$ is supported natively in the library --- you -don't need to define it yourself. The constant `PI` is a `Magnitude` _instance_. It's based on the -(natively included) irrational magnitude base, `Pi`. (Concretely: `PI` is defined as -`Magnitude{}`, in accordance with option 2 above.) +Because of its importance for angular variables, $\pi$ is supported natively in the library, via the +irrational magnitude base, `Pi`. To define a magnitude _instance_ for $\pi$, you can write: + +```cpp +constexpr auto PI = Magnitude{}; +``` If you need to represent an irrational number which can't be formed via any product of powers of the existing `Magnitude` types --- namely, integers and $\pi$ --- then you can define a new irrational @@ -124,8 +124,9 @@ To extract the value of a `Magnitude` instance `m` into a given numeric type `T` - The **computation** gets performed _at compile time_ in `long double`, giving extra precision. - The **result** gets cast to `float` and stored as a program constant. - Thus, `get_value(pow<3>(PI))` will be much more accurate than storing $\pi$ in a `float`, - and cubing it --- yet, there will be no loss in performance. + Thus, if you have a magnitude instance `PI`, then `get_value(pow<3>(PI))` will be much + more accurate than storing $\pi$ in a `float`, and cubing it --- yet, there will be no loss in + runtime performance. ### Checking for representability diff --git a/release/common_test_cases.hh b/release/common_test_cases.hh index 9bff3013..43218aad 100644 --- a/release/common_test_cases.hh +++ b/release/common_test_cases.hh @@ -19,6 +19,8 @@ // should pass. namespace au { +namespace { +constexpr auto PI = Magnitude{}; TEST(CommonSingleFile, HasExpectedUnits) { EXPECT_EQ(meters(1.23).in(meters), 1.23); @@ -48,4 +50,5 @@ TEST(CommonSingleFile, IncludesMathFunctions) { EXPECT_DOUBLE_EQ(sin(radians(get_value(PI / mag<2>()))), 1.0); } +} // namespace } // namespace au