Skip to content

Commit

Permalink
Deprecate PI (#250)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
chiphogg authored Jun 24, 2024
1 parent bd36cc5 commit 50de6e9
Show file tree
Hide file tree
Showing 16 changed files with 39 additions and 22 deletions.
2 changes: 2 additions & 0 deletions au/apply_magnitude_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ using ::testing::Not;
namespace au {
namespace detail {
namespace {
constexpr auto PI = Magnitude<Pi>{};

template <typename T>
std::vector<T> first_n_positive_values(std::size_t n) {
std::vector<T> result;
Expand Down
4 changes: 3 additions & 1 deletion au/constant_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ using ::testing::StrEq;
namespace au {
namespace {

constexpr auto PI = Magnitude<Pi>{};

template <typename U, typename R>
std::string stream_to_string(Quantity<U, R> q) {
std::ostringstream oss;
Expand Down Expand Up @@ -60,7 +62,6 @@ struct PlancksConstant : decltype(Joules{} * Seconds{} * H_JS) {
};
constexpr auto plancks_constant = QuantityMaker<PlancksConstant>{};
constexpr auto h = make_constant(plancks_constant);
} // namespace

TEST(MakeConstant, MakesConstantFromUnit) {
StaticAssertTypeEq<decltype(make_constant(SpeedOfLight{})), Constant<SpeedOfLight>>();
Expand Down Expand Up @@ -282,4 +283,5 @@ TEST(CanStoreValueIn, ChecksRangeOfTypeForIntegers) {
EXPECT_FALSE(decltype(c)::can_store_value_in<int16_t>(meters / second));
}

} // namespace
} // namespace au
4 changes: 4 additions & 0 deletions au/conversion_policy_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
#include "gtest/gtest.h"

namespace au {
namespace {

constexpr auto PI = Magnitude<Pi>{};

struct Grams : UnitImpl<Mass> {};
struct Kilograms : decltype(Grams{} * pow<3>(mag<10>())) {};
Expand Down Expand Up @@ -138,4 +141,5 @@ TEST(ConstructionPolicy, OkForIntegralRepAndEquivalentUnit) {
(ConstructionPolicy<EquivalentToDegrees, int8_t>::PermitImplicitFrom<Degrees, int>::value));
}

} // namespace
} // namespace au
4 changes: 3 additions & 1 deletion au/magnitude.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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<Pi>{};
[[deprecated(
"If you need a magnitude instance for pi, define your own as `constexpr auto PI = "
"Magnitude<Pi>{};`")]] static constexpr auto PI = Magnitude<Pi>{};
#endif

template <typename... BP1s, typename... BP2s>
Expand Down
4 changes: 3 additions & 1 deletion au/magnitude_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,12 @@ using ::testing::StaticAssertTypeEq;

namespace au {
namespace {
constexpr auto PI = Magnitude<Pi>{};

template <typename T, typename = std::enable_if_t<std::is_arithmetic<T>::value>>
constexpr T cubed(T x) {
return x * x * x;
}
} // namespace

TEST(Magnitude, SupportsEqualityComparison) {
constexpr auto mag_1 = mag<1>();
Expand Down Expand Up @@ -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 {

Expand Down
2 changes: 1 addition & 1 deletion au/rep_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ TEST(IsValidRep, TrueForStdComplex) {

TEST(IsValidRep, FalseForMagnitude) {
EXPECT_FALSE(IsValidRep<decltype(mag<84>())>::value);
EXPECT_FALSE(IsValidRep<decltype(sqrt(PI))>::value);
EXPECT_FALSE(IsValidRep<decltype(sqrt(Magnitude<Pi>{}))>::value);
}

TEST(IsValidRep, FalseForUnits) {
Expand Down
2 changes: 1 addition & 1 deletion au/units/test/degrees_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ namespace au {
TEST(Degrees, HasExpectedLabel) { expect_label<Degrees>("deg"); }

TEST(Degrees, RoughlyEquivalentToPiOver180Radians) {
EXPECT_DOUBLE_EQ(degrees(1.0).in(radians), get_value<double>(PI / mag<180>()));
EXPECT_DOUBLE_EQ(degrees(1.0).in(radians), get_value<double>(Magnitude<Pi>{} / mag<180>()));
}

TEST(Degrees, One360thOfARevolution) { EXPECT_EQ(degrees(360), revolutions(1)); }
Expand Down
2 changes: 1 addition & 1 deletion au/units/test/radians_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ namespace au {
TEST(Radians, HasExpectedLabel) { expect_label<Radians>("rad"); }

TEST(Radians, TwoPiPerRevolution) {
EXPECT_DOUBLE_EQ(radians(get_value<double>(mag<2>() * PI)).in(revolutions), 1.0);
EXPECT_DOUBLE_EQ(radians(get_value<double>(mag<2>() * Magnitude<Pi>{})).in(revolutions), 1.0);
}

TEST(Radians, HasExpectedSymbol) {
Expand Down
2 changes: 2 additions & 0 deletions au/wrapper_operations_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ namespace au {
namespace detail {
namespace {

constexpr auto PI = Magnitude<Pi>{};

template <typename Unit>
struct UnitWrapper : MakesQuantityFromNumber<UnitWrapper, Unit>,
ScalesQuantity<UnitWrapper, Unit>,
Expand Down
4 changes: 2 additions & 2 deletions docs/discussion/implementation/vector_space.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<Pi>{} / mag<180>()`[^4].

[^4]: `PI / mag<180>()` expands to `Magnitude<Pow<Prime<2>, -2>, Pow<Prime<3>, -2>, Pi,
[^4]: `Magnitude<Pi>{} / mag<180>()` expands to `Magnitude<Pow<Prime<2>, -2>, Pow<Prime<3>, -2>, Pi,
Pow<Prime<5>, -1>>`.

### Units
Expand Down
2 changes: 1 addition & 1 deletion docs/howto/new-units.md
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ Here are some example unit expressions we might reach for to define various comm
- Newtons: `Kilo<Grams>{} * Meters{} / squared(Seconds{})`
- Miles: `Feet{} * mag<5280>()`
- Degrees: `Radians{} * PI / mag<180>()`
- Degrees: `Radians{} * Magnitude<Pi>{} / mag<180>()`
## Aliases vs. strong types: best practices
Expand Down
6 changes: 3 additions & 3 deletions docs/install.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<Pi>{} / mag<180>();
```

These will "work", in the sense of producing correct results. But these ad hoc unit definitions
Expand Down
2 changes: 1 addition & 1 deletion docs/reference/corresponding_quantity.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ following example.
??? example "Example of 'two-hop' conversion, continued from above"

```cpp
MyDegrees angle = radians(get_value<double>(PI / mag<2>()));
MyDegrees angle = radians(get_value<double>(Magnitude<Pi>{} / mag<2>()));
```

Here we have a "two-hop" conversion. The corresponding quantity for `MyDegrees` is
Expand Down
1 change: 0 additions & 1 deletion docs/reference/detail/monovalue_types.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ Here are some canonical examples in Au.
|------|----------|------------|
| `Zero` | `ZERO` | Comparing to any `Quantity` |
| `Magnitude<>` | `ONE` | <ul><li>Equality comparison with other Magnitudes</li><li>`get_value<T>(ONE)`</li></ul> |
| `Magnitude<Pi>` | `PI` | <ul><li>Equality comparison with other Magnitudes</li><li>`get_value<T>(PI)`</li></ul> |
| `Radians` (and other units) | `Radians{}` (no special pre-formed instance) | Arithmetic with other units, such as `Radians{} / Meters{}` |

## Switching between types and values {#switching}
Expand Down
17 changes: 9 additions & 8 deletions docs/reference/magnitude.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<>`.
Expand Down Expand Up @@ -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<Pi>{}`, 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<Pi>{};
```

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
Expand Down Expand Up @@ -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<float>(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<float>(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

Expand Down
3 changes: 3 additions & 0 deletions release/common_test_cases.hh
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
// should pass.

namespace au {
namespace {
constexpr auto PI = Magnitude<Pi>{};

TEST(CommonSingleFile, HasExpectedUnits) {
EXPECT_EQ(meters(1.23).in(meters), 1.23);
Expand Down Expand Up @@ -48,4 +50,5 @@ TEST(CommonSingleFile, IncludesMathFunctions) {
EXPECT_DOUBLE_EQ(sin(radians(get_value<double>(PI / mag<2>()))), 1.0);
}

} // namespace
} // namespace au

0 comments on commit 50de6e9

Please sign in to comment.