Make Magnitude sum N-ary and more general #652
Conversation
When we start collecting like terms as part of adding `Constant` instances to each other, we will naturally form N-ary sums. To prepare the way, we introduce a `MagSum<Ms...>` utility to perform this operation. Now, we _could_ just implement this in terms of the existing `operator+` for `Magnitude`. However, that gets a little weird: it makes the result depend on the order. It's possible for a subset of inputs to overflow (thus making the binary operation return an error), when subsequent inputs might "rescue" the result (say, by subtracting to bring it back into range). Thus, I redid the implementation from scratch, using modular arithmetic for the result, and keeping track of the (signed) number of overflows. This gives us exact information about the true sum (over a very very wide range of values), although we can't always _use_ this information. Specifically, we can only use the result when the overflow is 0 (simple), or when it is -1 _and_ the sum is not 0 (because if sum _were_ 0 in this case it would actually represent -2^64). This new, more symmetrical implementation immediately takes the place of all three `Magnitude`-`Magnitude` implementations of `operator+`. Helps #607.
| static constexpr std::uint64_t sum = compute().sum; | ||
| static constexpr int overflow = compute().overflow; |
There was a problem hiding this comment.
Is the compiler smart enough not to do this twice?
There was a problem hiding this comment.
I was wondering about that myself. Ultimately I landed on "I'm curious, but it doesn't matter". Since we store the result(s) as constexpr variables, and then only ever use those, the worst that could happen is a little extra compile time. (And I do mean "little" --- I really don't see how it could be very much.)
Matter of fact... an earlier iteration just used compute() everywhere, so we wouldn't have to add definitions for static member variables down below! 😆
| // The surprising `sum == 0u` avoids asking for `mag<0>()`. | ||
| // It's fine, because it can never actually be used. |
There was a problem hiding this comment.
Is this just because the compiler can't intuit this invariant?
There was a problem hiding this comment.
The precise problem we would have if we deleted + (sum == 0u) is that whenever we take the first branch of the conditional, it amounts to std::conditional_t<true, Zero, decltype(mag<0>())>. Obviously, this "should" always just be Zero... but the fact that we have formed mag<0>() at all, anywhere, is enough for the compiler to bail out.
Co-authored-by: Michael Hordijk <hordijk@aurora.tech>
When we start collecting like terms as part of adding
Constantinstances to each other, we will naturally form N-ary sums. To prepare
the way, we introduce a
MagSum<Ms...>utility to perform thisoperation.
Now, we could just implement this in terms of the existing
operator+for
Magnitude. However, that gets a little weird: it makes the resultdepend on the order. It's possible for a subset of inputs to overflow
(thus making the binary operation return an error), when subsequent
inputs might "rescue" the result (say, by subtracting to bring it back
into range). Thus, I redid the implementation from scratch, using
modular arithmetic for the result, and keeping track of the (signed)
number of overflows. This gives us exact information about the true sum
(over a very very wide range of values), although we can't always use
this information. Specifically, we can only use the result when the
overflow is 0 (simple), or when it is -1 and the sum is not 0 (because
if sum were 0 in this case it would actually represent -2^64).
This new, more symmetrical implementation immediately takes the place of
all three
Magnitude-Magnitudeimplementations ofoperator+.Helps #607.