From 3d4d6fc7060da9f985c583552d08cd18940a55e5 Mon Sep 17 00:00:00 2001 From: Yves Delley Date: Sat, 20 Jul 2024 10:53:07 +0200 Subject: [PATCH 1/7] change implementation of converting constructor of quantity_point from another QP to leverage sudo_cast --- src/core/include/mp-units/framework/quantity_point.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/include/mp-units/framework/quantity_point.h b/src/core/include/mp-units/framework/quantity_point.h index aa4763d63..29e29a2f7 100644 --- a/src/core/include/mp-units/framework/quantity_point.h +++ b/src/core/include/mp-units/framework/quantity_point.h @@ -206,6 +206,7 @@ class quantity_point { quantity_point(quantity_point&&) = default; ~quantity_point() = default; + // explicit converting constructor from a quantity - only for quantity_points of `default_point_origin` template requires QuantityOf, quantity_spec> && std::constructible_from && (point_origin == default_point_origin(R)) && (implicitly_convertible(Q::quantity_spec, quantity_spec)) @@ -213,12 +214,14 @@ class quantity_point { { } + // construction from a quantity and matching origin template requires QuantityOf, quantity_spec> && std::constructible_from constexpr quantity_point(Q&& q, decltype(PO)) : quantity_from_origin_is_an_implementation_detail_(std::forward(q)) { } + // construction from a quantity and a related origin template requires Quantity> && std::constructible_from && ReferenceOf::reference)>, PO2::quantity_spec> && @@ -230,19 +233,16 @@ class quantity_point { { } + // converting constructor from a quantity_point of related origin template QP> requires std::constructible_from // NOLINTNEXTLINE(google-explicit-constructor, hicpp-explicit-conversions) constexpr explicit(!std::convertible_to) quantity_point(const QP& qp) : - quantity_from_origin_is_an_implementation_detail_([&] { - if constexpr (point_origin == QP::point_origin) - return qp.quantity_ref_from(point_origin); - else - return qp - point_origin; - }()) + quantity_point(detail::sudo_cast(qp)) { } + // converting constructor from a quantity_point of matching origin template requires(quantity_point_like_traits::point_origin == point_origin) && std::convertible_to< From e129b2228927af9c8d347c39c345ddd8cae20bb8 Mon Sep 17 00:00:00 2001 From: Yves Delley Date: Sat, 20 Jul 2024 12:30:08 +0200 Subject: [PATCH 2/7] change internal strategy to alwasy use the output unit --- src/core/include/mp-units/bits/sudo_cast.h | 60 ++++++++++++------- .../mp-units/framework/quantity_point.h | 7 ++- test/static/quantity_point_test.cpp | 17 ++++-- 3 files changed, 59 insertions(+), 25 deletions(-) diff --git a/src/core/include/mp-units/bits/sudo_cast.h b/src/core/include/mp-units/bits/sudo_cast.h index fd7a99739..3b47b70ba 100644 --- a/src/core/include/mp-units/bits/sudo_cast.h +++ b/src/core/include/mp-units/bits/sudo_cast.h @@ -136,11 +136,27 @@ template [[nodiscard]] constexpr QuantityPoint auto sudo_cast(FromQP&& qp) { using qp_type = std::remove_reference_t; - if constexpr (is_same_v, - std::remove_const_t>) { + using traits = magnitude_conversion_traits; + if constexpr (ToQP::point_origin == qp_type::point_origin) { + // no change of offset needed return quantity_point{ sudo_cast(std::forward(qp).quantity_from(qp_type::point_origin)), - qp_type::point_origin}; + ToQP::point_origin}; + } else if constexpr (qp_type::quantity_type::unit == ToQP::quantity_type::unit) { + // no scaling of the unit is needed; thus we can perform all computation in a single unit without any runtime + // scaling anywhere + using offset_rep_type = typename traits::c_rep_type; + // in the following, we take the detour through `quantity_point` to determine the offset between the two + // point_origins; there seem to be cases where we have two `zeroeth_point_origins` of different units (i.e. m vs. + // km), where the subtraction for the latter is not defined. + static constexpr auto zero = offset_rep_type{0} * qp_type::reference; + // we statically convert the offset to unit of the quantities, to avoid runtime rescaling. + // TODO: should we implement do a rounding instead here? + static constexpr auto offset = sudo_cast>( + quantity_point{zero, qp_type::point_origin} - quantity_point{zero, ToQP::point_origin}); + return quantity_point{ + sudo_cast(std::forward(qp).quantity_from(qp_type::point_origin) + offset), + ToQP::point_origin}; } else { // it's unclear how hard we should try to avoid truncation here. For now, the only corner case we cater for, // is when the range of the quantity type of at most one of QP or ToQP doesn't cover the offset between the @@ -153,23 +169,27 @@ template // In the following, we carefully select the order of these three operations: each of (a) and (b) is scheduled // either before or after (c), such that (c) acts on the largest range possible among all combination of source // and target unit and represenation. - using traits = magnitude_conversion_traits; - using c_rep_type = typename traits::c_rep_type; - if constexpr (traits::num_mult * traits::irr_mult > traits::den_mult) { - // original unit had a larger unit magnitude; if we first convert to the common representation but retain the - // unit, we obtain the largest possible range while not causing truncation of fractional values. This is optimal - // for the offset computation. - return sudo_cast( - sudo_cast>(std::forward(qp)) - .point_for(ToQP::point_origin)); - } else { - // new unit may have a larger unit magnitude; we first need to convert to the new unit (potentially causing - // truncation, but no more than if we did the conversion later), but make sure we keep the larger of the two - // representation types. Then, we can perform the offset computation. - return sudo_cast(sudo_cast>(std::forward(qp)) - .point_for(ToQP::point_origin)); - } + // + // This implementation handles operations (a) and (b) by delegating to the `sudo_cast`, + // while opertaion (c) is delegated to quantity::operator+. We ensure that no hidden conversions happen in the + // latter, by proving both operands in the same representation, the `intermediate_quantity_type`. The + // `intermediate_quantity_type` in turn is chosen such that operations (a) and (b) individually happen either during + // the inital conversion from the input, or during the final conversion to the output, whichever is optimal given + // the input and output types. + static constexpr auto intermediate_reference = make_reference(qp_type::quantity_spec, ToQP::unit); + // We always work in the larger of the two (input/output) representations. + using intermediate_rep_type = typename traits::c_type; + using intermediate_quantity_type = quantity; + // in the following, we take the detour through `quantity_point` to determine the offset between the two + // point_origins; there seem to be cases where we have two `zeroeth_point_origins` of different units (i.e. m vs. + // km), where the subtraction for the latter is not defined. + static constexpr auto zero = intermediate_rep_type{0} * intermediate_reference; + static constexpr intermediate_quantity_type offset = + quantity_point{zero, qp_type::point_origin} - quantity_point{zero, ToQP::point_origin}; + return quantity_point{ + sudo_cast( + sudo_cast(std::forward(qp).quantity_from(qp_type::point_origin)) + offset), + ToQP::point_origin}; } } diff --git a/src/core/include/mp-units/framework/quantity_point.h b/src/core/include/mp-units/framework/quantity_point.h index 29e29a2f7..1985f29a6 100644 --- a/src/core/include/mp-units/framework/quantity_point.h +++ b/src/core/include/mp-units/framework/quantity_point.h @@ -238,7 +238,12 @@ class quantity_point { requires std::constructible_from // NOLINTNEXTLINE(google-explicit-constructor, hicpp-explicit-conversions) constexpr explicit(!std::convertible_to) quantity_point(const QP& qp) : - quantity_point(detail::sudo_cast(qp)) + quantity_from_origin_is_an_implementation_detail_([&] { + if constexpr (point_origin == QP::point_origin) + return qp.quantity_ref_from(point_origin); + else + return qp - point_origin; + }()) { } diff --git a/test/static/quantity_point_test.cpp b/test/static/quantity_point_test.cpp index 06ad1f16a..39a689ec9 100644 --- a/test/static/quantity_point_test.cpp +++ b/test/static/quantity_point_test.cpp @@ -1747,13 +1747,22 @@ static_assert(value_cast>(quantity_point{2 * isq: static_assert(value_cast>(quantity_point{2000 * isq::height[m]}) .quantity_from_origin_is_an_implementation_detail_.numerical_value_in(km) == 2); // a value_cast which includes a change to the point origin -static_assert(value_cast>(quantity_point{2000 * isq::height[m], - ground_level}) +static_assert(value_cast>(quantity_point{int{2000} * isq::height[m], + ground_level}) + .quantity_from_origin_is_an_implementation_detail_.numerical_value_in(m) == 2042); +// a value_cast which includes a change to the point origin and the representation +static_assert(value_cast>(quantity_point{2000 * isq::height[m], + ground_level}) .quantity_from_origin_is_an_implementation_detail_.numerical_value_in(m) == 2042); // a value_cast which includes a change to the point origin as-well as a change in units -static_assert(value_cast>(quantity_point{2 * isq::height[km], - ground_level}) +static_assert(value_cast>(quantity_point{int{2} * isq::height[km], + ground_level}) + .quantity_from_origin_is_an_implementation_detail_.numerical_value_in(m) == 2042); +// a value_cast which includes a change to the point origin as-well as a change in units and the representation +static_assert(value_cast>(quantity_point{2 * isq::height[km], + ground_level}) .quantity_from_origin_is_an_implementation_detail_.numerical_value_in(m) == 2042); + // a value_cast which changes all three of unit, rep, point_origin simultaneously, and the range of either FromQP or // ToQP does not include the other's point_origin static_assert(value_cast>( From ee7595044c66ebfcfd183550ab66fbe61a65115d Mon Sep 17 00:00:00 2001 From: Yves Delley Date: Tue, 23 Jul 2024 12:20:16 +0200 Subject: [PATCH 3/7] WIP --- src/core/include/mp-units/bits/sudo_cast.h | 87 ++++++++++++++++++---- 1 file changed, 74 insertions(+), 13 deletions(-) diff --git a/src/core/include/mp-units/bits/sudo_cast.h b/src/core/include/mp-units/bits/sudo_cast.h index 3b47b70ba..bd5fc7612 100644 --- a/src/core/include/mp-units/bits/sudo_cast.h +++ b/src/core/include/mp-units/bits/sudo_cast.h @@ -73,6 +73,8 @@ struct magnitude_conversion_traits { static constexpr multiplier_type den_mult = val(den); static constexpr multiplier_type irr_mult = val(irr); static constexpr multiplier_type ratio = num_mult / den_mult * irr_mult; + // TODO: This may not be the right choice here; see #580 and #599 + static constexpr bool use_floating_point_scaling = std::is_floating_point_v; }; @@ -102,13 +104,14 @@ template } else { // scale the number using traits = magnitude_conversion_traits>; - if constexpr (std::is_floating_point_v) { - // this results in great assembly + if constexpr (traits::use_floating_point_scaling) { + // this results in great assembly for floating-point types auto res = static_cast( static_cast(q.numerical_value_is_an_implementation_detail_) * traits::ratio); return {res, To::reference}; } else { - // this is slower but allows conversions like 2000 m -> 2 km without loosing data + // this avoids floating-point operations and thus guarantees exact conversion results + // where possible (like 2000 m -> 2 km), but may be slower for floating-point types. auto res = static_cast( static_cast(q.numerical_value_is_an_implementation_detail_) * traits::num_mult / traits::den_mult * traits::irr_mult); @@ -137,27 +140,85 @@ template { using qp_type = std::remove_reference_t; using traits = magnitude_conversion_traits; + using c_rep_type = typename traits::c_rep_type; + static constexpr auto output_unit_ref = make_reference(qp_type::quantity_spec, ToQP::unit); + static constexpr auto offset_represented_as = [](auto quantity) { + using Q = decltype(quantity); + // in the following, we take the detour through `quantity_point` to determine the offset between the two + // point_origins; there seem to be cases where we have two `zeroeth_point_origins` of different units (i.e. m vs. + // km), and the subtraction operator between such two origin points seems to be missing. + auto zero = Q::rep{0} * Q::reference; + // TODO: should we attempt to round values here, if needed? + return sudo_cast(quantity_point{zero, qp_type::point_origin} - quantity_point{zero, ToQP::point_origin}); + }; if constexpr (ToQP::point_origin == qp_type::point_origin) { - // no change of offset needed + // no change of offset needed; delegate to the pure sudo_cast implementation return quantity_point{ sudo_cast(std::forward(qp).quantity_from(qp_type::point_origin)), ToQP::point_origin}; } else if constexpr (qp_type::quantity_type::unit == ToQP::quantity_type::unit) { // no scaling of the unit is needed; thus we can perform all computation in a single unit without any runtime // scaling anywhere - using offset_rep_type = typename traits::c_rep_type; - // in the following, we take the detour through `quantity_point` to determine the offset between the two - // point_origins; there seem to be cases where we have two `zeroeth_point_origins` of different units (i.e. m vs. - // km), where the subtraction for the latter is not defined. - static constexpr auto zero = offset_rep_type{0} * qp_type::reference; // we statically convert the offset to unit of the quantities, to avoid runtime rescaling. - // TODO: should we implement do a rounding instead here? - static constexpr auto offset = sudo_cast>( - quantity_point{zero, qp_type::point_origin} - quantity_point{zero, ToQP::point_origin}); + static constexpr auto offset = offset_represented_as(quantity{}); return quantity_point{ sudo_cast(std::forward(qp).quantity_from(qp_type::point_origin) + offset), ToQP::point_origin}; + } else if constexpr (traits::use_floating_point_scaling) { + // for the provided set of representation types, we will use floating-point intermediate representations + // (typically, when at least one of input or output representations is floating-point). + // with those, the choice of unit has almost no impact on the conversion accuracy, and thus we can choose + // the larger unit of the two (input/output) to ensure there is no risk of overflow. + static constexpr auto intermediate_reference = []() { + if constexpr (traits::num_mult * traits::irr_mult >= traits.den_mult) { + // the input unit is larger + return qp_type::reference; + } else { + return output_unit_ref; + } + }(); + using intermediate_rep_type = typename traits::c_type; + using intermediate_quantity_type = quantity; + static constexpr auto offset = offset_represented_as(intermediate_quantity_type{}); + return quantity_point{ + sudo_cast( + sudo_cast(std::forward(qp).quantity_from(qp_type::point_origin)) + offset), + ToQP::point_origin}; } else { + // with integral representations, we expect the conversion result to be accurate to the resolution of the output + // representation. So, in general, we should perform the offset computation using the output units. However, + // if the offset is large compared to the output unit, there is a risk of overflow. Usually, this requires + // that the offset is specified in terms of a larger unit (because otherwise, the offset would overflow too). + // Therefore, we use the offset's unit as intermediate unit if the offset falls outside of the range of the + // smaller unit. + static constexpr auto intermediate_reference = []() { + if constexpr (traits::num_mult * traits::irr_mult >= traits.den_mult) { + // the output unit is smaller; check if we can represent the offset faithfully in the output unit without + // overflow + using candidate_offset_type = quantity; + constexpr auto min_representable_offset = value_cast(candidate_offset_type::min()); + constexpr auto max_representable_offset = value_cast(candidate_offset_type::max()); + constexpr auto offset_value = offset_represented_at>(); + if constexpr ((min_representable_offset <= offset_value) && (offset_value <= max_representable_offset)) { + // the offset can reasonably be represented by the output unit, so we use that one + return output_unit_ref; + } else { + // the offset would overflow with expressed in the output unit; use the input unit instead + return qp_type::reference; + } + } else { + // the output units is larger; we always use that one + return output_unit_ref; + } + }(); + using intermediate_rep_type = typename traits::c_type; + using intermediate_quantity_type = quantity; + static constexpr auto offset = offset_represented_as(intermediate_quantity_type{}); + return quantity_point{ + sudo_cast( + sudo_cast(std::forward(qp).quantity_from(qp_type::point_origin)) + offset), + ToQP::point_origin}; + // it's unclear how hard we should try to avoid truncation here. For now, the only corner case we cater for, // is when the range of the quantity type of at most one of QP or ToQP doesn't cover the offset between the // point origins. In that case, we need to be careful to ensure we use the quantity type with the larger range @@ -176,7 +237,7 @@ template // `intermediate_quantity_type` in turn is chosen such that operations (a) and (b) individually happen either during // the inital conversion from the input, or during the final conversion to the output, whichever is optimal given // the input and output types. - static constexpr auto intermediate_reference = make_reference(qp_type::quantity_spec, ToQP::unit); + static constexpr auto intermediate_reference = output_unit_ref; // We always work in the larger of the two (input/output) representations. using intermediate_rep_type = typename traits::c_type; using intermediate_quantity_type = quantity; From afc69a6e34bf31ebe348a20d857f063dcf9aacd5 Mon Sep 17 00:00:00 2001 From: Yves Delley Date: Sat, 14 Sep 2024 18:05:47 +0200 Subject: [PATCH 4/7] fix C++20 compatibility (no static constexpr local variables) --- src/core/include/mp-units/bits/sudo_cast.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/include/mp-units/bits/sudo_cast.h b/src/core/include/mp-units/bits/sudo_cast.h index deb89a0f0..e8169cd2e 100644 --- a/src/core/include/mp-units/bits/sudo_cast.h +++ b/src/core/include/mp-units/bits/sudo_cast.h @@ -161,8 +161,8 @@ template; - static constexpr auto output_unit_ref = make_reference(FromQP::quantity_spec, ToQP::unit); - static constexpr auto offset_represented_as = [](auto quantity) { + constexpr auto output_unit_ref = make_reference(FromQP::quantity_spec, ToQP::unit); + constexpr auto offset_represented_as = [](auto quantity) { using Q = decltype(quantity); // in the following, we take the detour through `quantity_point` to determine the offset between the two // point_origins; there seem to be cases where we have two `zeroeth_point_origins` of different units (i.e. m vs. @@ -181,7 +181,7 @@ template{}); + constexpr auto offset = offset_represented_as(quantity{}); return quantity_point{ sudo_cast(std::forward(qp).quantity_from(FromQP::point_origin) + offset), ToQP::point_origin}; @@ -190,7 +190,7 @@ template= value_traits::den_mult) { // the input unit is larger return FromQP::reference; @@ -200,7 +200,7 @@ template; - static constexpr auto offset = offset_represented_as(intermediate_quantity_type{}); + constexpr auto offset = offset_represented_as(intermediate_quantity_type{}); return quantity_point{ sudo_cast( sudo_cast(std::forward(qp).quantity_from(FromQP::point_origin)) + offset), @@ -212,7 +212,7 @@ template= value_traits::den_mult) { // the output unit is smaller; check if we can represent the offset faithfully in the output unit without // overflow @@ -234,7 +234,7 @@ template; - static constexpr auto offset = offset_represented_as(intermediate_quantity_type{}); + constexpr auto offset = offset_represented_as(intermediate_quantity_type{}); return quantity_point{ sudo_cast( sudo_cast(std::forward(qp).quantity_from(FromQP::point_origin)) + offset), From 61198fe44cd69fcf6cb7890f2f422a36e5fb2d6d Mon Sep 17 00:00:00 2001 From: Yves Delley Date: Sat, 14 Sep 2024 18:07:33 +0200 Subject: [PATCH 5/7] fix small merge regression --- src/core/include/mp-units/bits/sudo_cast.h | 2 +- src/core/include/mp-units/framework/quantity_point.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/include/mp-units/bits/sudo_cast.h b/src/core/include/mp-units/bits/sudo_cast.h index e8169cd2e..c55e0c0b7 100644 --- a/src/core/include/mp-units/bits/sudo_cast.h +++ b/src/core/include/mp-units/bits/sudo_cast.h @@ -157,7 +157,7 @@ template; - using c_rep_type = type_traits::c_rep_typ; + using c_rep_type = type_traits::c_rep_type; using multiplier_type = typename type_traits::multiplier_type; using value_traits = conversion_value_traits; diff --git a/src/core/include/mp-units/framework/quantity_point.h b/src/core/include/mp-units/framework/quantity_point.h index 17af1c25f..31cb22037 100644 --- a/src/core/include/mp-units/framework/quantity_point.h +++ b/src/core/include/mp-units/framework/quantity_point.h @@ -228,7 +228,7 @@ class quantity_point { constexpr quantity_point(FwdQ&& q, PO2) : quantity_point( quantity_point::reference, PO2{}, typename std::remove_reference_t::rep>{ - std::forward(q), PO2{}}) + std::forward(q), PO2{}}) { } From b52020d5922b01c9ec7d2de4a47996a5f38e2fb2 Mon Sep 17 00:00:00 2001 From: Yves Delley Date: Sun, 15 Sep 2024 18:32:55 +0200 Subject: [PATCH 6/7] make it compile again --- src/core/include/mp-units/bits/sudo_cast.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/include/mp-units/bits/sudo_cast.h b/src/core/include/mp-units/bits/sudo_cast.h index c55e0c0b7..f145a92fe 100644 --- a/src/core/include/mp-units/bits/sudo_cast.h +++ b/src/core/include/mp-units/bits/sudo_cast.h @@ -162,7 +162,7 @@ template; constexpr auto output_unit_ref = make_reference(FromQP::quantity_spec, ToQP::unit); - constexpr auto offset_represented_as = [](auto quantity) { + constexpr auto offset_represented_as = [&](auto quantity) { using Q = decltype(quantity); // in the following, we take the detour through `quantity_point` to determine the offset between the two // point_origins; there seem to be cases where we have two `zeroeth_point_origins` of different units (i.e. m vs. @@ -190,7 +190,7 @@ template= value_traits::den_mult) { // the input unit is larger return FromQP::reference; @@ -212,14 +212,14 @@ template= value_traits::den_mult) { // the output unit is smaller; check if we can represent the offset faithfully in the output unit without // overflow using candidate_offset_type = quantity; constexpr auto min_representable_offset = value_cast(candidate_offset_type::min()); constexpr auto max_representable_offset = value_cast(candidate_offset_type::max()); - constexpr auto offset_value = offset_represented_at>(); + constexpr auto offset_value = offset_represented_as(quantity{}); if constexpr ((min_representable_offset <= offset_value) && (offset_value <= max_representable_offset)) { // the offset can reasonably be represented by the output unit, so we use that one return output_unit_ref; From 584ac9f3f4acaae4b490a02ffbf27c5a95d7948e Mon Sep 17 00:00:00 2001 From: Yves Delley Date: Sun, 15 Sep 2024 21:23:43 +0200 Subject: [PATCH 7/7] fixes --- test/static/quantity_point_test.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/test/static/quantity_point_test.cpp b/test/static/quantity_point_test.cpp index 7f80b21c9..95326297b 100644 --- a/test/static/quantity_point_test.cpp +++ b/test/static/quantity_point_test.cpp @@ -1759,9 +1759,15 @@ static_assert(value_cast>(qu ground_level}) .quantity_from_origin_is_an_implementation_detail_.numerical_value_in(m) == 2042); // a value_cast which includes a change to the point origin as-well as a change in units and the representation -static_assert(value_cast>(quantity_point{2 * isq::height[km], - ground_level}) - .quantity_from_origin_is_an_implementation_detail_.numerical_value_in(m) == 2042); +template +constexpr T cxpr_abs(T v) +{ + return v < T{0} ? -v : v; +} +static_assert(cxpr_abs(value_cast>( + quantity_point{2 * isq::height[km], ground_level}) + .quantity_from_origin_is_an_implementation_detail_.numerical_value_in(m) - + 2042.) < 1.e-7); // a value_cast which changes all three of unit, rep, point_origin simultaneously, and the range of either FromQP or // ToQP does not include the other's point_origin