FYI: You might be converting float->int wrong

[Peter Kasting]

If you never work with floating-point values in C++, you can ignore this.

TLDR: If in doubt, you want base::ClampRound().

All the following ways of converting a float to an int are wrong:

float f = ...;
int i1 = static_cast<int>(f);  // Wrong (1)

int i2 = std::floor(f);  // Wrong (2)

int i3 = std::round(f);  // Also wrong (2)

int i4 = static_cast<int>(f + 0.5f);  // Wrong (3)

int i5 = base::saturated_cast<int>(f + ((f > 0) ? 0.5f : -0.5f));  // Wrong (4)

Here's why:

1. floats can represent values outside the range of an int, so implicit-casting or static_casting is unsafe due to potential overflow. If you want "truncate"-like behavior, use base::saturated_cast.
2. std::ceil/floor/round() do not return ints, they return floating-point types, so the bullet above still applies. If you want behaviors like these, use base::ClampCeil/Floor/Round().
3. Trying to round manually by adding 0.5 and truncating is generally the wrong behavior for negative numbers. Plus, the bullets above still apply. Use base::ClampRound().
4. Due to floating-point precision loss, manually adding 0.5 and compensating for negative numbers by flipping the sign is STILL wrong for certain edge cases (IIRC, 0.5 - 1ULP). IEEE754 is hard. Use base::ClampRound().

Thank you, you can stop here if you want.

EXTRA BONUS NERD-OUT

ALL of these are slow in hardware and exhibit statistical bias compared to std::nearbyint(), which is "round using the current hardware rounding mode, which is likely 'round to nearest, break ties towards even'" (like std::round(), except values ending in exactly .5 go to the nearest even number instead of away from zero). Of course this returns a floating-point number so you still need base::saturated_cast. Don't use std::rint() (which is like std::nearbyint() but can raise a floating-point exception), and don't use std::lrint() or std::llrint() (because they return long and long long, which we don't allow, and because you're pinky-promising not to overflow, which is less safe than just guaranteeing the code is robust).

PK

[Daniel Cheng]

Thanks for the writeup! Is it worth adding some PRESUBMIT checks for std::rint(), std::lrint() and std::llrint()?

Daniel

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFC0Rrs1Lx0qH8XUPMYo2k27iq5mnjhOR8FEAe6tx4DMQQ%40mail.gmail.com.

[Peter Kasting]

On Thu, Oct 26, 2023 at 4:53 PM Daniel Cheng <dch...@chromium.org> wrote:

Thanks for the writeup! Is it worth adding some PRESUBMIT checks for std::rint(), std::lrint() and std::llrint()?

I doubt it: https://source.chromium.org/search?q=-f:%5Ev8%2F%20-f:%5Eout%2F%20-f:%5Enative_client_sdk%2F%20std::l%3Fl%3Frint%5C(%20-file:third_party&ss=chromium

PK