I am not sure if the value_cast<Q>(qp) is really needed? Anyway, this example needs to be fixed ;-)

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.

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.

Three use-cases for value_cast<Q>:

1. You want guarantee there is no inadverted change of point_origin when going from FromQP to ToQP typedefs or template arguments you may have. Writing value_cast<typename ToQP::quantity_type>(qp) is much shorter than quantity_point{value_cast<typename ToQP::quantity_type>(qp.quantity_from(FromQP::point_origin)), FromQP::point_origin} and also safer
2. You know something about FromQP and ToQP that allows you to choose a good order of calculations among {change reference point, change unit, change representation}. Implementing that, you may get back to use-cast 1) (i.e. casting the rest, not touching the point_origin).
3. You want to implement a generic FromQP to ToQP, like my new sudo_cast<ToQP> implementation. As it turns out, my proposed "intelligent" sudo_cast<QP> is in fact just two instances of case 2): Depending on the involved types, I need to choose a specific order.

In general, in embedded use cases, we typically care a lot about representation types and may want to control each aspect separately. I checked, I'm already using that value_cast<Q>(qp) quite a number of times. Often, sensors would work with values referencing some quite arbitrary reference point; unless calculations interface "external" quantity_point types, I try not to touch the reference point because of truncation issues, and just juggle the Q part of the representation - even though the values do represent quantity points all the time.

Writing value_cast(qp) is much shorter than quantity_point{value_cast(qp.quantity_from(FromQP::point_origin)), FromQP::point_origin} and also safer

Why not to write: quantity_point{ToQP::unit, typename ToQP::rep>(qp}? I know that it is still spelling two things instead of one but I would like to avoid suggesting people that they should make aliases for quantity_point and quantity for the same abstraction to make it work. Not everyone will be smart enough or restricting themselves to just use typename QP::quantity_type as you mentioned above.

But if you think it is the only way to make it work, then please commit it to the repo. I just don't know how to get feedback about it before putting this under standardization. We will not be able to easily verify how many people depend on this feature in their code. Probably for ISO I will not propose it in the first run, and add it later if people will request for it.

To me, there is elegance in the symmetry that value cast guarantees to only change the representation (with potential for rounding, underflow and overflow, but no fundamental change of meaning), and it takes basically whatever you throw (assuming it makes sense). For a q, you can today either throw a Q, a U or a rep, and it all makes sense. For a qp, you can now throw a QP, a U and a rep. Why not a Q? The're all sensible and clear.

For standardisation, I think you can easily specify the U, rep and Q overloads of value_cast(qp) in terms of the QP overload (or in terms of the corresponding value_cast(q) overloads), so it adds very little complexity.

That said, I don't need it to make it work. The current implementation of value_cast<QP> delegates to the new sudo_cast<QP>, whose implementation currently only uses sudo_casts (and .point_for, which I possibly should fix - re your other comment). So we absolutely don't need value_cast<Q>(qp) in the API.

OK, let's try it then 😃

Can we reuse sude_cast here? I wouldn't like to duplicate the implementation. Maybe sudo_cast could benefit from new logic as well?

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.

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.

@burnpanck, I hope that you successfully delivered the upgraded project on time. Do you still work on this PR?

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.

On it. I'm currently refactoring the implementation of the new value_cast<QP> into a new overload of sudo_cast, sharing the multiplier calculation between the two sudo_cast overloads as you prefer. While doing so, I noticed a few details on the existing sudo_cast<Q> that appear disputable to me. I may open a separate issue to review that with you, but for now, I'll just follow by analogy.

As far as the code sharing between the two sudo_cast overloads, regarding

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.

The shared code is purely compile-time; all of the runtime behaviour of sudo_cast<Q> was already re-used within my previous value_cast<QP> implementation through the call to value_cast<Q>. Now, the new sudo_cast<QP> will share the compile-time computation through a separate helper, and re-use the runtime code through a call to sudo_cast<Q>. sudo_cast<Q> will have most of it's compile-time guts ripped out into that shared helper, but it's runtime-path will remain untouched.

I didn't mean the final result might be wrong. It never was and we unit test for it as well.

What I meant here is that the same conversion may compile into more assembly instructions than a similar operation on a fundamental type. I do not know how to unit test this, and I always discover an issue only while preparing some slides to my talks that prove that there is not overhead over using double. Unfortunately, I often find this to not be true anymore in this case and have to fix the sudo cast.

This is why I would like you to check the assembly before merging. Alternatively, we can merge it, and then verify in the Compiler Explorer and fix things if the codegen will be worse.

Sure, but I did not touch the original sudo_cast implementation of that multiplication, and the sudo_cast<QP> implementation does not do any unit conversion on it's own. It fully delegates to that to the existing sudo_cast<Q>.

Sure! This is why I asked to have one place where those things are implemented. Initially, you copy-pasted it to another file, which would make it much harder to maintain and ensure synergy and correctness.

value_cast<typename ToQP::quantity_type>(qp) is still a bit controversial 😉

And even if not, then we first have to describe what it does and how to use it (e.g., QP::quantity_type) before we use it in the description of value_cast<ToQP>(qp).

Ok, let me do another take on the documentation, this one here is indeed a bit short.

I think you are assuming here an integral representation type, which often will not be the case as double is the default in the library. In cases of a floating point representation type, we should calculate the entire conversion factor in one step and then do only one multiplication operation on the value to get the result. Otherwise, we will not generate assembly equivalent to operations on double.

If my previous comment was unclear, I will just note that the relative_point_origin stores an offset, which has its own unit and representation as well. We could factor in this conversion here as well (instead of calling point_for() as a separate step).

However, maybe it would be too complex to do it here anyway and the measurable performance gains would not be that big.

Indeed, those considerations about order of the conversion operations are most relevant for integral types, where both underflow and overflow are likely. It doesn't hurt for double either. However, I'm not sure what you mean by "calculate the entire conversion factor in one step". A change of point-origin requires an addition. A change in unit requires a multiplication. This implementation here does at most one multiplication and at most one addition in all cases. Here is how:

• The initial check for same point-origin directly delegates to the sudo_cast<Q> implementation to select the "best" way to do that multiplication, and avoids the addition
• The two code-paths for a change in point origin go as follos:
  • The first path for where the unit gets smaller and thus the number gets larger:
    1. cast to the intermediate representation type using sudo_cast (no-op in the case of floatingpoints). previously, that was a value_cast, but I figured that within a sudo_cast, value_cast is off limit.
    2. add/subtract the change in reference using .point_for; hopefully, the implementation of .point_for is such that this is a single addition and no multiplication
    3. scale the unit/number and
    4. cast the to the final representation type using sudo_cast.
  • The second path for where the unit gets larger and thus the number will get smaller (or stay the same):
    1. cast to the intermediate representation type and
    2. scale the unit/number using sudo_cast. If the intermediate representation type is the same as the source erpresentation and the units remain the same, this is a no_op.
    3. add/subtract the change in reference using .point_for
    4. cast to the final representation type using sudo_cast. No-op for floating-point types.

So I believe this implementation is as efficient as it can be through a careful selection of types. While both code-paths invoke sudo_cast twice, there is always one where the source and target units match and no multiplication is needed. For floating-point, there is also not cast, so that cast is a no-op. Furthermore, the implementation here does never do any arithmetic itself - it delegates the addition to .point_for and the multiplication to sudo_cast<Q>. Personally, I would even prefer to call value_cast instead, because in each of the two code-paths, one of the casts is a pure representation cast, which would be more clear as value_cast<rep>.

Wait, now I understand you are indicating that .point_for may potentially change the unit and representation type? In that case, you are of course right, we should replace the .point_for call here with a dedicated implementation.

However, I'm not sure what you mean by "calculate the entire conversion factor in one step".

I mean something like this:

  if constexpr (std::is_floating_point_v<multiplier_type>) {
      // this results in great assembly
      constexpr auto ratio = val(num) / val(den) * val(irr);
     // use precalculated ratio...
  }
  else // ...

Precalculation of ratio as a constexpr variable ensures that the assembly will include only one multiplication by this constant. The else branch typically results in more assembly instructions.