Skip to content

Commit

Permalink
Compute magnitudes in the "real part" of a type
Browse files Browse the repository at this point in the history
A `Magnitude` is _defined_ to be a _real_ number.  But users can ask for
its value in, say, `std::complex<int>` (either explicitly or
implicitly).  This is a problem, because in computing that value, we
check for overflow, which involves less-than comparisons... but complex
numbers are not ordered, so there is no meaningful sense of `<`!

To fix this, we introduce the notion of `RealPart<T>`, a type trait that
gives a type related to `T` that does have a notion of comparability.
(Since `Magnitude` is defined to be a real number, calling it "real
part" is a good fit.)  `RealPart<T>` is just `T` in almost all cases,
but it becomes "the type of `T{}.real()`" whenever that exists.  This
lets us support both `std::complex` and complex number types from other
libraries.

The core of the fix was to make sure all of our `Magnitude` operations
are taking place in `RealPart<T>` rather than `T`.  This basically boils
down to the _call_ to `base_power_value`, and the _implementations_ for
`SafeCastingChecker`.

This change also has knock-on effects for implicit conversion policy.
It turns out the old implementation of `CoreImplicitConversionPolicy`
was always silently assuming that the type was already a real number
type. Therefore, we rename it by adding an `AssumingReal` suffix on the
end. The new `CoreImplicitConversionPolicy` passes `RealPart<Rep>` along
to this, after first checking for a new pitfall to guard against:
namely, implicitly converting a complex type to a real type.
  • Loading branch information
chiphogg committed Aug 3, 2024
1 parent 3c267a3 commit 4e98aeb
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 8 deletions.
21 changes: 19 additions & 2 deletions au/code/au/conversion_policy.hh
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ template <typename U1, typename U2>
struct SameDimension : stdx::bool_constant<U1::dim_ == U2::dim_> {};

template <typename Rep, typename ScaleFactor, typename SourceRep>
struct CoreImplicitConversionPolicyImpl
struct CoreImplicitConversionPolicyImplAssumingReal
: stdx::disjunction<
std::is_floating_point<Rep>,
stdx::conjunction<std::is_integral<SourceRep>,
Expand All @@ -61,7 +61,24 @@ struct CoreImplicitConversionPolicyImpl

// Always permit the identity scaling.
template <typename Rep>
struct CoreImplicitConversionPolicyImpl<Rep, Magnitude<>, Rep> : std::true_type {};
struct CoreImplicitConversionPolicyImplAssumingReal<Rep, Magnitude<>, Rep> : std::true_type {};

// `SettingPureRealFromMixedReal<A, B>` tests whether `A` is a pure real type, _and_ `B` is a type
// that has a real _part_, but is not purely real (call it a "mixed-real" type).
//
// The point is to guard against situations where we're _implicitly_ converting a "mixed-real" type
// (i.e., typically a complex number) to a pure real type.
template <typename Rep, typename SourceRep>
struct SettingPureRealFromMixedReal
: stdx::conjunction<stdx::negation<std::is_same<SourceRep, RealPart<SourceRep>>>,
std::is_same<Rep, RealPart<Rep>>> {};

template <typename Rep, typename ScaleFactor, typename SourceRep>
struct CoreImplicitConversionPolicyImpl
: stdx::conjunction<stdx::negation<SettingPureRealFromMixedReal<Rep, SourceRep>>,
CoreImplicitConversionPolicyImplAssumingReal<RealPart<Rep>,
ScaleFactor,
RealPart<SourceRep>>> {};

template <typename Rep, typename ScaleFactor, typename SourceRep>
using CoreImplicitConversionPolicy = CoreImplicitConversionPolicyImpl<Rep, ScaleFactor, SourceRep>;
Expand Down
26 changes: 26 additions & 0 deletions au/code/au/conversion_policy_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

#include "au/conversion_policy.hh"

#include <complex>

#include "au/unit_of_measure.hh"
#include "gtest/gtest.h"

Expand Down Expand Up @@ -106,6 +108,17 @@ TEST(ImplicitRepPermitted, FunctionalInterfaceWorksAsExpected) {
EXPECT_TRUE(implicit_rep_permitted_from_source_to_target<float>(Grams{}, Kilograms{}));
}

TEST(ImplicitRepPermitted, HandlesComplexRep) {
// These test cases are the same as the ones in `FunctionalInterfaceWorksAsExpected`, except
// that we replace the target type `T` with `std::complex<T>`.
EXPECT_TRUE(
implicit_rep_permitted_from_source_to_target<std::complex<int>>(Kilograms{}, Grams{}));
EXPECT_FALSE(
implicit_rep_permitted_from_source_to_target<std::complex<int>>(Grams{}, Kilograms{}));
EXPECT_TRUE(
implicit_rep_permitted_from_source_to_target<std::complex<float>>(Grams{}, Kilograms{}));
}

TEST(ConstructionPolicy, PermitImplicitFromWideVarietyOfTypesForFloatingPointTargets) {
using gigagrams_float_policy = ConstructionPolicy<Gigagrams, float>;
EXPECT_TRUE((gigagrams_float_policy::PermitImplicitFrom<Grams, int>::value));
Expand All @@ -126,6 +139,19 @@ TEST(ConstructionPolicy, PermitsImplicitFromIntegralTypesIffTargetScaleDividesSo
EXPECT_FALSE((grams_int_policy::PermitImplicitFrom<Milligrams, int>::value));
}

TEST(ConstructionPolicy, ComplexToRealPreventsImplicitConversion) {
// `complex<int>` -> `float`: forbid, although `int` -> `float` is allowed.
using gigagrams_float_policy = ConstructionPolicy<Gigagrams, float>;
ASSERT_TRUE((gigagrams_float_policy::PermitImplicitFrom<Grams, int>::value));
EXPECT_FALSE((gigagrams_float_policy::PermitImplicitFrom<Grams, std::complex<int>>::value));

// (`int` or `complex<int>`) -> `complex<float>`: both allowed.
using gigagrams_complex_float_policy = ConstructionPolicy<Gigagrams, std::complex<float>>;
EXPECT_TRUE((gigagrams_complex_float_policy::PermitImplicitFrom<Grams, int>::value));
EXPECT_TRUE(
(gigagrams_complex_float_policy::PermitImplicitFrom<Grams, std::complex<int>>::value));
}

TEST(ConstructionPolicy, ForbidsImplicitConstructionOfIntegralTypeFromFloatingPtType) {
using grams_int_policy = ConstructionPolicy<Grams, int>;
EXPECT_FALSE((grams_int_policy::PermitImplicitFrom<Grams, double>::value));
Expand Down
27 changes: 21 additions & 6 deletions au/code/au/magnitude.hh
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#pragma once

#include <limits>
#include <utility>

#include "au/packs.hh"
#include "au/power_aliases.hh"
Expand Down Expand Up @@ -467,12 +468,26 @@ constexpr bool all(const bool (&values)[N]) {
return true;
}

// `RealPart<T>` is `T` itself, unless that type has a `.real()` member.
template <typename T>
using TypeOfRealMember = decltype(std::declval<T>().real());
template <typename T>
// `RealPartImpl` is basically equivalent to the `detected_or<T, TypeOfRealMember, T>` part at the
// end. But we special-case `is_arithmetic` to get a fast short-circuit for the overwhelmingly most
// common case.
struct RealPartImpl : std::conditional<std::is_arithmetic<T>::value,
T,
stdx::experimental::detected_or_t<T, TypeOfRealMember, T>> {
};
template <typename T>
using RealPart = typename RealPartImpl<T>::type;

template <typename Target, typename Enable = void>
struct SafeCastingChecker {
template <typename T>
constexpr bool operator()(T x) {
return stdx::cmp_less_equal(std::numeric_limits<Target>::lowest(), x) &&
stdx::cmp_greater_equal(std::numeric_limits<Target>::max(), x);
return stdx::cmp_less_equal(std::numeric_limits<RealPart<Target>>::lowest(), x) &&
stdx::cmp_greater_equal(std::numeric_limits<RealPart<Target>>::max(), x);
}
};

Expand All @@ -481,8 +496,8 @@ struct SafeCastingChecker<Target, std::enable_if_t<std::is_integral<Target>::val
template <typename T>
constexpr bool operator()(T x) {
return std::is_integral<T>::value &&
stdx::cmp_less_equal(std::numeric_limits<Target>::lowest(), x) &&
stdx::cmp_greater_equal(std::numeric_limits<Target>::max(), x);
stdx::cmp_less_equal(std::numeric_limits<RealPart<Target>>::lowest(), x) &&
stdx::cmp_greater_equal(std::numeric_limits<RealPart<Target>>::max(), x);
}
};

Expand All @@ -501,8 +516,8 @@ constexpr MagRepresentationOrError<T> get_value_result(Magnitude<BPs...>) {
}

// Force the expression to be evaluated in a constexpr context.
constexpr auto widened_result =
product({base_power_value<T, ExpT<BPs>::num, static_cast<std::uintmax_t>(ExpT<BPs>::den)>(
constexpr auto widened_result = product(
{base_power_value<RealPart<T>, ExpT<BPs>::num, static_cast<std::uintmax_t>(ExpT<BPs>::den)>(
BaseT<BPs>::value())...});

if ((widened_result.outcome != MagRepresentationOutcome::OK) ||
Expand Down
12 changes: 12 additions & 0 deletions au/code/au/quantity_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,18 @@ TEST(Quantity, SupportsDivisionOfRealQuantityIntoComplexCoefficient) {
EXPECT_THAT(quotient.imag(), DoubleEq(4.0));
}

TEST(Quantity, SupportsConvertingUnitsForComplexQuantity) {
constexpr auto a = meters(std::complex<double>{-3.0, 4.0});
const auto b = a.as(centi(meters));
EXPECT_THAT(b, SameTypeAndValue(centi(meters)(std::complex<double>{-300.0, 400.0})));
}

TEST(Quantity, SupportsExplicitRepConversionToComplexRep) {
constexpr auto a = feet(15'000.0);
const auto b = a.coerce_as<std::complex<int>>(miles);
EXPECT_THAT(b, SameTypeAndValue(miles(std::complex<int>{3, 0})));
}

TEST(Quantity, CanDivideArbitraryQuantities) {
constexpr auto d = feet(6.);
constexpr auto t = hours(3.);
Expand Down

0 comments on commit 4e98aeb

Please sign in to comment.