Add overflow helpers for applying rational to int #212

chiphogg · 2023-12-15T16:44:05Z

C++'s integer promotion rules for small types make this specific use
case --- applying a rational magnitude to an integral type --- really
interesting! The product type can be bigger than the type of the two
inputs, so the overflow might not happen when you think it should.
Moreover, the fact that we subsequently divide by a denominator means
that we can re-enter the valid range and still produce a correct
result!

The only way I know how to deal with something this complicated is to
make a separate target that does only that one thing. Happily, it's
still the case that there is some maximum and minimum value for each
type that will not overflow for a particular rational magnitude. The
new helper target exists to figure out what those limits are.

I exposed this limitation by adding a conversion between meters and
yards in uint16_t. This is a great test case because the unit ratio
between meters and yards is very close to 1 (it's 1143 to 1250), meaning
the denominator division "rescues" the great majority of cases.
Moreover, the multiplicative factor (either 1143 or 1250 depending on
direction) is far less than the ratio between uint16_t and its
promoted type, assuming the latter is equivalent to int32_t. So,
despite the fact that we're multiplying by a number which is pretty
large relative to the type, we almost never actually overflow!

I fixed up the test case so that if it does fail (say, we add some new
unit conversion that exposes a new logic error), it will be very easy to
understand why.

C++'s integer promotion rules for small types make this specific use case --- applying a rational magnitude to an integral type --- really interesting! The product type can be bigger than the type of the two inputs, so the overflow might not happen when you think it should. _Moreover,_ the fact that we subsequently divide by a denominator means that we can _re-enter_ the valid range and still produce a correct result! The only way I know how to deal with something this complicated is to make a separate target that does only that one thing. Happily, it's still the case that there is some maximum and minimum value for each type that will not overflow for a particular rational magnitude. The new helper target exists to figure out what those limits are. I exposed this limitation by adding a conversion between meters and yards in `uint16_t`. This is a great test case because the unit ratio between meters and yards is very close to 1 (it's 1143 to 1250), meaning the denominator division "rescues" the great majority of cases. Moreover, the multiplicative factor (either 1143 or 1250 depending on direction) is far less than the ratio between `uint16_t` and its promoted type, assuming the latter is equivalent to `int32_t`. So, despite the fact that we're multiplying by a number which is pretty large relative to the type, we almost never actually overflow! I fixed up the test case so that if it _does_ fail (say, we add some new unit conversion that exposes a new logic error), it will be very easy to understand why.

chiphogg · 2023-12-15T16:54:37Z

By the way: here's what we would get if we only applied the changes to quantity_test.cc onto main:

[ RUN      ] IsConversionLossy.CorrectlyDiscriminatesBetweenLossyAndLosslessConversions
au/quantity_test.cc:777: Failure
Expected equality of these values:
  is_lossy
    Which is: true
  did_value_change
    Which is: false
Conversion is lossy (overflows), but round-trip conversion did not change the value.  original: 1250 yd, converted: 1143 m, round_trip: 1250 yd
au/quantity_test.cc:777: Failure
Expected equality of these values:
  is_lossy
    Which is: true
  did_value_change
    Which is: false
Conversion is lossy (overflows), but round-trip conversion did not change the value.  original: 2500 yd, converted: 2286 m, round_trip: 2500 yd
au/quantity_test.cc:777: Failure
Expected equality of these values:
  is_lossy
    Which is: true
  did_value_change
    Which is: false

[... many, many rows omitted ...]

au/quantity_test.cc:777: Failure
Expected equality of these values:
  is_lossy
    Which is: true
  did_value_change
    Which is: false
Conversion is lossy (overflows), but round-trip conversion did not change the value.  original: 63750 yd, converted: 58293 m, round_trip: 63750 yd
au/quantity_test.cc:777: Failure
Expected equality of these values:
  is_lossy
    Which is: true
  did_value_change
    Which is: false
Conversion is lossy (overflows), but round-trip conversion did not change the value.  original: 65000 yd, converted: 59436 m, round_trip: 65000 yd
[  FAILED  ] IsConversionLossy.CorrectlyDiscriminatesBetweenLossyAndLosslessConversions (63 ms)

So, the current state of main calls certain yards-to-meters conversions lossy, even though the library produces a lossless result. With the present PR's changes, these tests all pass!

geoffviola

The production code looks better. The test coverage has improved around many tricky cases. Thanks!

geoffviola · 2023-12-18T20:48:12Z

au/apply_rational_magnitude_to_integral_test.cc

+    constexpr auto huge = pow<400>(mag<10>()) / mag<3>();
+
+    {
+        int max_int = 123;


Q: It looks like the value 123 is always written over. Is there any reason for this value vs 0?

Nope: I just needed something that is clearly not 0. 😅

geoffviola · 2023-12-18T20:54:52Z

au/apply_magnitude.hh

@@ -141,13 +164,12 @@ struct ApplyMagnitudeImpl<Mag, ApplyAs::RATIONAL_MULTIPLY, T, true> {
                  "Mismatched instantiation (should never be done manually)");

    constexpr T operator()(const T &x) {
-        return x * get_value<T>(numerator(Mag{})) / get_value<T>(denominator(Mag{}));
+        return static_cast<T>(x * get_value<T>(numerator(Mag{})) /


Commend: Nice catch!

Actually, in chatting about the logic with you, I noticed a bug, and it was here! Fortunately, it was pretty self-contained, and easy to write a test case for. Check out 81c4a3b for the fix.

Great! I was thinking about static_cast<T>. I guess there's also the tricky bit about get_value<P>. This line is trickier than it looks 😅

This is an edge case that I had missed before. The rational magnitude applier helper libraries all check whether `N` and `D` can fit in the _promoted_ type of `T`, not `T` itself. This is what they _should_ do, because this is the true limit of what's possible in the computation. However, on the _implementation_ side, we were getting the value in the type `T`. I was able to write a test case that exposed this, and now the test case passes. We should not need to worry about this for the other `ApplyMagnitude` specializations, because the `x * N / D` is the only case we have where the limits of the promoted type can produce a different outcome than the limits of the target type.

chiphogg added the release notes: 🐛 lib (bugfix) PR fixing a defect in the library code label Dec 15, 2023

chiphogg requested a review from geoffviola December 15, 2023 16:44

chiphogg added 2 commits December 15, 2023 11:50

Print unit labels if test fails

1d991b7

Try fixing Wconversion error on gcc

9a28dd7

Fix some incorrect comments

6b593d7

geoffviola approved these changes Dec 18, 2023

View reviewed changes

chiphogg merged commit a771f57 into main Dec 18, 2023
9 checks passed

chiphogg deleted the chiphogg/rational-overflow-checker#110 branch December 18, 2023 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add overflow helpers for applying rational to int #212

Add overflow helpers for applying rational to int #212

chiphogg commented Dec 15, 2023

chiphogg commented Dec 15, 2023

geoffviola left a comment

geoffviola Dec 18, 2023

chiphogg Dec 18, 2023

geoffviola Dec 18, 2023

chiphogg Dec 18, 2023

geoffviola Dec 18, 2023

Add overflow helpers for applying rational to int #212

Add overflow helpers for applying rational to int #212

Conversation

chiphogg commented Dec 15, 2023

chiphogg commented Dec 15, 2023

geoffviola left a comment

Choose a reason for hiding this comment

geoffviola Dec 18, 2023

Choose a reason for hiding this comment

chiphogg Dec 18, 2023

Choose a reason for hiding this comment

geoffviola Dec 18, 2023

Choose a reason for hiding this comment

chiphogg Dec 18, 2023

Choose a reason for hiding this comment

geoffviola Dec 18, 2023

Choose a reason for hiding this comment