Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MathUtil: Add SaturatingCast to cast floats more safely #9480

Merged
merged 3 commits into from Apr 6, 2021

Conversation

leoetlino
Copy link
Member

A modified version of #8871 with the clamp function added to MathUtil along with tests.

The second commit makes VertexLoaderTest use it in order to avoid undefined behavior:

A prvalue of a floating-point type can be converted to a prvalue of
an integer type. The conversion truncates; that is, the fractional
part is discarded. The behavior is undefined if the truncated value
cannot be represented in the destination type. ([conv.fpint]/1)

@JosJuice
Copy link
Member

Could you remove this in favor of the new SaturatingCast?

constexpr auto clamped_cast = [](size_t x) {
return static_cast<unsigned int>(
std::min<size_t>(std::numeric_limits<unsigned int>().max(), x));
};

@leoetlino leoetlino force-pushed the saturating-cast branch 2 times, most recently from 55005cc to d9c9571 Compare January 28, 2021 16:28
@leoetlino
Copy link
Member Author

@JosJuice Done. The tests also pass now; apparently using std::clamp was not a good idea because it forces value, lo and hi to be the same type T and converting lo/hi to T can already result in overflow.

@Dentomologist
Copy link
Contributor

Dentomologist commented Feb 11, 2021

SaturatingCast<unsigned int>(-1)

returns 4294967295 instead of 0, which I don't think is intended.

@sepalani
Copy link
Contributor

@Dentomologist
You're right and this should produce a compiler warning signed/unsigned mismatch. Maybe a static_assert can be added to prevent edgy cases like that.

@leoetlino leoetlino force-pushed the saturating-cast branch 2 times, most recently from 65fcf08 to efa7b49 Compare February 11, 2021 21:46
@leoetlino leoetlino marked this pull request as ready for review February 11, 2021 21:55
if (value < 0)
return lo;
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like there should be a static_assert(lo == 0) or something like that here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or perhaps replace return lo; with return 0;? What do you think would be clearer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think the static_assert would be clearer, since it makes the u32(...) < 0 assumption more explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Works for me

@leoetlino leoetlino force-pushed the saturating-cast branch 2 times, most recently from 09682dc to 5c2b9fb Compare April 6, 2021 21:19
[conv.fpint]/1:

> A prvalue of a floating-point type can be converted to a prvalue of
> an integer type. The conversion truncates; that is, the fractional
> part is discarded. The behavior is undefined if the truncated value
> cannot be represented in the destination type.
@leoetlino leoetlino merged commit c161746 into dolphin-emu:master Apr 6, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants