implement value_cast<ToQ> and value_cast<ToQP> #571
Conversation
burnpanck
force-pushed
the
feature/more-value-casts
branch
from
5b6df8b
to
3bd4f4d
Compare
|
BTW, instead of |
burnpanck
force-pushed
the
feature/more-value-casts
branch
from
77113cb
to
c51baae
Compare
…rigin in value_cast, to prevent overflow in more cases.
burnpanck
changed the title
|
Given that it is somewhat nontrivial to implement a correct general |
|
Oh, shall I try to sqash? |
…ude_type_impl when both types are the same
| * using ToQP = quantity_point<mm, B, int>; | ||
| * auto qp = value_cast<ToQP>(quantity_point{1.23 * m}); |
There was a problem hiding this comment.
I am not sure if the value_cast<Q>(qp) is really needed? Anyway, this example needs to be fixed ;-)
There was a problem hiding this comment.
Anyway, this example needs to be fixed
Right, indeed obvious copy-paste error.
I am not sure if the
value_cast<Q>(qp)is really needed?
value_cast<Q>(qp) operations are convenient as part of implementations of value_cast<QP>(qp)-like operations (see below). While my implementation of the latter tries to select the best possible implementation based on the provided types, an expert user may know more about the value range that is in fact being used in their case and instead want to implement a different order of operations. Then, they would also like to be able to use value_cast<Q>(qp). Finally, it provides some symmetry to value_cast<Q>(q). On the other hand, one could argue that value_cast<Q>(qp) misleadingly suggests that the result type is going to be Q, which of course it is not. However, that same argument could be made for the value_cast<Repr>(...) overloads. Anyway, I don't have all too strong feelings here, we can also drop it.
There was a problem hiding this comment.
I do not think it is needed and it may be misleading indeed.
value_cast<Q>(q) makes sense as someone may have a typedef for quantity and does not want to extract a unit and rep all the time for a cast. The same applies to quantity_point.
On the other hand, value_cast<Q>(qp) is strange, as one should not have a quantity typedef for an abstraction modeled through a quantity_point.
| 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)); |
There was a problem hiding this comment.
Can we reuse sude_cast here? I wouldn't like to duplicate the implementation. Maybe sudo_cast could benefit from new logic as well?
There was a problem hiding this comment.
sudo_cast is being used (indirectly), through the two value_cast. The apparent duplication is just in order to determine the conversion factor to be applied to the numeric value. The runtime code in the body here does not apply that conversion factor at all; but it needs to know that factor is eventually going to make number larger or smaller. That is needed in turn to decide when to do that conversion: if the number gets larger, we want to postpone until after the offset adjustment, and if it gets smaller, we want to do it before the offset adjustment.
We could probably create a second overload of sudo_cast which operates on quantity_point (basically the content of this value_cast, minus the constraint on matching quantity_specs, and ensuring that the quantity_spec is being casted correctly). Then, we could extract that calculation of the rescaling factor into a helper function and use it from both overloads.
There was a problem hiding this comment.
I would prefer that. I've already learned that sudo_cast is really fragile, and any changes should be verified against the codegen in Compiler Explorer. I would not like to have nearly copy pasted implementation in a separate file because it would be hard to maintain and keep consistent.
There was a problem hiding this comment.
@burnpanck, I hope that you successfully delivered the upgraded project on time. Do you still work on this PR?
There was a problem hiding this comment.
The refactor is done, but we had to postpone a few features, which we are still delivering as we speak. I do intend to finish this one tough, as our code is already heavily reliant on these value_cast variants.
There was a problem hiding this comment.
@burnpanck, I plan to release mp-units 2.2 in the following days. Please let me know if you will have time to finish it soon.
Fixes #568