Skip to content

Conversation

@chiphogg
Copy link
Member

The main hurdle here is that for signed integral types, the minimum
value is more negative than the max value is positive. To deal with
this, we add a clamped_negate(T) function, which negates the input,
but saturates at the limits of the type. We use it consistently in a
few other places where it makes sense.

We also add an equal function to the MagHelper, so that we can
seamlessly check whether a value in a given type happens to equal a
Magnitude. This again helps us deal with this hurdle that we have
with signed integral types.

With these helpers in hand, we can implement MaxGood using a very
similar strategy as for MinGood. The only substantive difference is
that MinGood automatically returns 0 for all unsigned types period,
but MaxGood only does this for an unsigned type with a negative scale
factor
.

Helps #349.

The main hurdle here is that for signed integral types, the minimum
value is more negative than the max value is positive.  To deal with
this, we add a `clamped_negate(T)` function, which negates the input,
but saturates at the limits of the type.  We use it consistently in a
few other places where it makes sense.

We also add an `equal` function to the `MagHelper`, so that we can
seamlessly check whether a value in a given type happens to equal a
`Magnitude`.  This again helps us deal with this hurdle that we have
with signed integral types.

With these helpers in hand, we can implement `MaxGood` using a very
similar strategy as for `MinGood`.  The only substantive difference is
that `MinGood` automatically returns `0` for all unsigned types period,
but `MaxGood` only does this for an unsigned type _with a negative scale
factor_.

Helps #349.
@chiphogg chiphogg requested a review from hoffbrinkle July 16, 2025 01:39
}

// Name reads as "highest of (limits divided by value)". Remember that the value can be negative,
// so we just take whichever limit is larger _after_ dividing. And since `Abs<M>` can be assumed to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was having trouble following this comment. The "large after dividing" only applies in the negative value case, correct?

Suggested change
// so we just take whichever limit is larger _after_ dividing. And since `Abs<M>` can be assumed to
// in which case we just take whichever limit is larger _after_ dividing. And since `Abs<M>` can be assumed to

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reword didn't seem quite right, because we always take the limit that is larger after dividing. I went back and gave the comment a more thorough rewrite.

Comment on lines +966 to +967
TEST(MultiplyTypeBy, MaxGoodForUnlimitedSignedTimesNegIntIsLowerLimitDivByMag) {
EXPECT_THAT(max_good_value(multiply_type_by<int8_t>(-mag<1>())), SameTypeAndValue(int8_t{127}));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man, this one is a bit of mind bender until I really thought about it. Having a use case for negative magnitudes really helped me reason about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole effort has been one mind bender after another! It was such a relief to get to a working proof of concept in my local client after multiple weeks of not knowing how the ideas would work in practice.

Also, I don't even want to think about how many lines of code we need in various places to deal with the fact that a signed integer's most negative value is further from zero than its most positive value. 😅

@chiphogg chiphogg merged commit 2758bfd into main Jul 16, 2025
14 checks passed
@chiphogg chiphogg deleted the chiphogg/overflow-max-good-multiply#349 branch July 16, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants