From 4e98aebdf8e1ba7c576297eefea866b022369df0 Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Sat, 3 Aug 2024 17:50:21 -0400 Subject: [PATCH] Compute magnitudes in the "real part" of a type A `Magnitude` is _defined_ to be a _real_ number. But users can ask for its value in, say, `std::complex` (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`, 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` 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` 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` along to this, after first checking for a new pitfall to guard against: namely, implicitly converting a complex type to a real type. --- au/code/au/conversion_policy.hh | 21 +++++++++++++++++++-- au/code/au/conversion_policy_test.cc | 26 ++++++++++++++++++++++++++ au/code/au/magnitude.hh | 27 +++++++++++++++++++++------ au/code/au/quantity_test.cc | 12 ++++++++++++ 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/au/code/au/conversion_policy.hh b/au/code/au/conversion_policy.hh index fe0de712..a48dbde1 100644 --- a/au/code/au/conversion_policy.hh +++ b/au/code/au/conversion_policy.hh @@ -52,7 +52,7 @@ template struct SameDimension : stdx::bool_constant {}; template -struct CoreImplicitConversionPolicyImpl +struct CoreImplicitConversionPolicyImplAssumingReal : stdx::disjunction< std::is_floating_point, stdx::conjunction, @@ -61,7 +61,24 @@ struct CoreImplicitConversionPolicyImpl // Always permit the identity scaling. template -struct CoreImplicitConversionPolicyImpl, Rep> : std::true_type {}; +struct CoreImplicitConversionPolicyImplAssumingReal, Rep> : std::true_type {}; + +// `SettingPureRealFromMixedReal` 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 +struct SettingPureRealFromMixedReal + : stdx::conjunction>>, + std::is_same>> {}; + +template +struct CoreImplicitConversionPolicyImpl + : stdx::conjunction>, + CoreImplicitConversionPolicyImplAssumingReal, + ScaleFactor, + RealPart>> {}; template using CoreImplicitConversionPolicy = CoreImplicitConversionPolicyImpl; diff --git a/au/code/au/conversion_policy_test.cc b/au/code/au/conversion_policy_test.cc index b1dde31e..1ce2ded8 100644 --- a/au/code/au/conversion_policy_test.cc +++ b/au/code/au/conversion_policy_test.cc @@ -14,6 +14,8 @@ #include "au/conversion_policy.hh" +#include + #include "au/unit_of_measure.hh" #include "gtest/gtest.h" @@ -106,6 +108,17 @@ TEST(ImplicitRepPermitted, FunctionalInterfaceWorksAsExpected) { EXPECT_TRUE(implicit_rep_permitted_from_source_to_target(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`. + EXPECT_TRUE( + implicit_rep_permitted_from_source_to_target>(Kilograms{}, Grams{})); + EXPECT_FALSE( + implicit_rep_permitted_from_source_to_target>(Grams{}, Kilograms{})); + EXPECT_TRUE( + implicit_rep_permitted_from_source_to_target>(Grams{}, Kilograms{})); +} + TEST(ConstructionPolicy, PermitImplicitFromWideVarietyOfTypesForFloatingPointTargets) { using gigagrams_float_policy = ConstructionPolicy; EXPECT_TRUE((gigagrams_float_policy::PermitImplicitFrom::value)); @@ -126,6 +139,19 @@ TEST(ConstructionPolicy, PermitsImplicitFromIntegralTypesIffTargetScaleDividesSo EXPECT_FALSE((grams_int_policy::PermitImplicitFrom::value)); } +TEST(ConstructionPolicy, ComplexToRealPreventsImplicitConversion) { + // `complex` -> `float`: forbid, although `int` -> `float` is allowed. + using gigagrams_float_policy = ConstructionPolicy; + ASSERT_TRUE((gigagrams_float_policy::PermitImplicitFrom::value)); + EXPECT_FALSE((gigagrams_float_policy::PermitImplicitFrom>::value)); + + // (`int` or `complex`) -> `complex`: both allowed. + using gigagrams_complex_float_policy = ConstructionPolicy>; + EXPECT_TRUE((gigagrams_complex_float_policy::PermitImplicitFrom::value)); + EXPECT_TRUE( + (gigagrams_complex_float_policy::PermitImplicitFrom>::value)); +} + TEST(ConstructionPolicy, ForbidsImplicitConstructionOfIntegralTypeFromFloatingPtType) { using grams_int_policy = ConstructionPolicy; EXPECT_FALSE((grams_int_policy::PermitImplicitFrom::value)); diff --git a/au/code/au/magnitude.hh b/au/code/au/magnitude.hh index f93e3cbd..cfed3da3 100644 --- a/au/code/au/magnitude.hh +++ b/au/code/au/magnitude.hh @@ -15,6 +15,7 @@ #pragma once #include +#include #include "au/packs.hh" #include "au/power_aliases.hh" @@ -467,12 +468,26 @@ constexpr bool all(const bool (&values)[N]) { return true; } +// `RealPart` is `T` itself, unless that type has a `.real()` member. +template +using TypeOfRealMember = decltype(std::declval().real()); +template +// `RealPartImpl` is basically equivalent to the `detected_or` 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::value, + T, + stdx::experimental::detected_or_t> { +}; +template +using RealPart = typename RealPartImpl::type; + template struct SafeCastingChecker { template constexpr bool operator()(T x) { - return stdx::cmp_less_equal(std::numeric_limits::lowest(), x) && - stdx::cmp_greater_equal(std::numeric_limits::max(), x); + return stdx::cmp_less_equal(std::numeric_limits>::lowest(), x) && + stdx::cmp_greater_equal(std::numeric_limits>::max(), x); } }; @@ -481,8 +496,8 @@ struct SafeCastingChecker::val template constexpr bool operator()(T x) { return std::is_integral::value && - stdx::cmp_less_equal(std::numeric_limits::lowest(), x) && - stdx::cmp_greater_equal(std::numeric_limits::max(), x); + stdx::cmp_less_equal(std::numeric_limits>::lowest(), x) && + stdx::cmp_greater_equal(std::numeric_limits>::max(), x); } }; @@ -501,8 +516,8 @@ constexpr MagRepresentationOrError get_value_result(Magnitude) { } // Force the expression to be evaluated in a constexpr context. - constexpr auto widened_result = - product({base_power_value::num, static_cast(ExpT::den)>( + constexpr auto widened_result = product( + {base_power_value, ExpT::num, static_cast(ExpT::den)>( BaseT::value())...}); if ((widened_result.outcome != MagRepresentationOutcome::OK) || diff --git a/au/code/au/quantity_test.cc b/au/code/au/quantity_test.cc index 939b9ebc..740ae6e0 100644 --- a/au/code/au/quantity_test.cc +++ b/au/code/au/quantity_test.cc @@ -463,6 +463,18 @@ TEST(Quantity, SupportsDivisionOfRealQuantityIntoComplexCoefficient) { EXPECT_THAT(quotient.imag(), DoubleEq(4.0)); } +TEST(Quantity, SupportsConvertingUnitsForComplexQuantity) { + constexpr auto a = meters(std::complex{-3.0, 4.0}); + const auto b = a.as(centi(meters)); + EXPECT_THAT(b, SameTypeAndValue(centi(meters)(std::complex{-300.0, 400.0}))); +} + +TEST(Quantity, SupportsExplicitRepConversionToComplexRep) { + constexpr auto a = feet(15'000.0); + const auto b = a.coerce_as>(miles); + EXPECT_THAT(b, SameTypeAndValue(miles(std::complex{3, 0}))); +} + TEST(Quantity, CanDivideArbitraryQuantities) { constexpr auto d = feet(6.); constexpr auto t = hours(3.);