better handle simultaneous change of representation, unit and point_origin in value_cast, to prevent overflow in more cases.

src/core/include/mp-units/framework/value_cast.h

@@ -231,10 +231,51 @@ template<QuantityPoint ToQP, typename QP>
            std::constructible_from<typename ToQP::rep, typename std::remove_reference_t<QP>::rep>
 [[nodiscard]] constexpr QuantityPoint auto value_cast(QP&& qp)
 {
-  return quantity_point{
-    value_cast<typename ToQP::quantity_type>(std::forward<QP>(qp).quantity_from_origin_is_an_implementation_detail_),
-    std::remove_reference_t<QP>::point_origin}
-    .point_for(ToQP::point_origin);
+  using qp_type = std::remove_reference_t<QP>;
+  if constexpr (is_same_v<std::remove_const_t<decltype(ToQP::point_origin)>, std::remove_const_t<decltype(qp_type::point_origin)>>) {
+    return quantity_point{
+      value_cast<typename ToQP::quantity_type>(std::forward<QP>(qp).quantity_from(qp_type::point_origin)),
+      qp_type::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
+    // point origins. In that case, we need to be careful to ensure we use the quantity type with the larger range
+    // of the two to perform the point_origin conversion.
+    // Numerically, we'll potentially need to do three things:
+    //  (a) cast the representation type
+    //  (b) scale the numerical value
+    //  (c) add/subtract the origin difference
+    // 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.
+    constexpr Magnitude auto c_mag = get_canonical_unit(qp_type::unit).mag / get_canonical_unit(ToQP::unit).mag;
+    constexpr Magnitude auto num = detail::numerator(c_mag);
+    constexpr Magnitude auto den = detail::denominator(c_mag);
+    constexpr Magnitude auto irr = c_mag * (den / num);
+    using c_rep_type = detail::maybe_common_type<typename ToQP::rep, typename qp_type::rep>;
+    using c_mag_type = detail::common_magnitude_type<c_mag>;
+    using multiplier_type = conditional<
+      treat_as_floating_point<c_rep_type>,
+      // ensure that the multiplier is also floating-point
+      conditional<std::is_arithmetic_v<value_type_t<c_rep_type>>,
+                  // reuse user's type if possible
+                  std::common_type_t<c_mag_type, value_type_t<c_rep_type>>, std::common_type_t<c_mag_type, double>>,
+      c_mag_type>;
+    constexpr auto val = [](Magnitude auto m) { return get_value<multiplier_type>(m); };
+    if constexpr (val(num) * val(irr) > val(den)) {
+      // 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 value_cast<typename ToQP::quantity_type>(
+        value_cast<c_rep_type>(std::forward<QP>(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 value_cast<typename ToQP::quantity_type>(
+        value_cast<ToQP::unit, c_rep_type>(std::forward<QP>(qp)).point_for(ToQP::point_origin));
+    }
+  }
 }
 
 

test/static/quantity_point_test.cpp

@@ -1695,21 +1695,25 @@ static_assert(value_cast<m, float>(lvalue_qp).quantity_from_zero().numerical_val
 static_assert(value_cast<quantity<km, int>>(quantity_point{2000 * m}).quantity_from_zero().numerical_value_in(km) == 2);
 static_assert(value_cast<quantity_point<km>>(quantity_point{2000 * m}).quantity_from_zero().numerical_value_in(km) ==
               2);
-static_assert(
-  !requires(quantity_point<isq::width[m]> qp) { value_cast<quantity<m>>(qp); },
-  "value_cast shall not cast between different quantity types");
-static_assert(
-  !requires(quantity_point<m> qp) { value_cast<quantity<isq::width[m]>>(qp); },
-  "value_cast shall not cast between different quantity types");
-static_assert(value_cast<quantity_point<m, mean_sea_level>>(quantity_point<km, ground_level>{2 * km})
-                .quantity_ref_from(mean_sea_level)
+
+template<typename ToQ, typename FromQ>
+constexpr bool value_cast_is_forbidden()
+{
+  // it appears we cannot have the requires clause right inside static_assert
+  return !requires(FromQ q) { value_cast<ToQ>(q); };
+}
+static_assert(value_cast_is_forbidden<quantity_point<m>, quantity_point<isq::width[m]>>(),
+              "value_cast shall not cast between different quantity types");
+static_assert(value_cast_is_forbidden<quantity_point<isq::width[m]>, quantity_point<m>>(),
+              "value_cast shall not cast between different quantity types");
+static_assert(value_cast<quantity_point<isq::height[m], mean_sea_level>>(quantity_point{2 * isq::height[km], ground_level})
+                .quantity_from_origin_is_an_implementation_detail_
                 .numerical_value_in(m) == 2042);
-static_assert(value_cast<quantity_point<cm, mean_sea_level, int>>(quantity_point<mm, ground_level, std::int8_t>{
-                                                                    std::int8_t{100} * mm})
-                .quantity_ref_from(mean_sea_level)
+static_assert(value_cast<quantity_point<isq::height[cm], mean_sea_level, int>>(quantity_point{std::int8_t{100} * isq::height[mm], ground_level})
+                .quantity_from_origin_is_an_implementation_detail_
                 .numerical_value_in(cm) == 4210);
-static_assert(value_cast<quantity_point<mm, ground_level, std::int8_t>>(quantity_point<cm, mean_sea_level>{4210 * cm})
-                .quantity_ref_from(ground_level)
+static_assert(value_cast<quantity_point<isq::height[mm], ground_level, std::int8_t>>(quantity_point{4210 * isq::height[cm], mean_sea_level})
+                .quantity_from_origin_is_an_implementation_detail_
                 .numerical_value_in(mm) == 100);