[webkit-reviews] review granted: [Bug 194263] clampTo(): do not convert the input to double when dealing with integers : [Attachment 361273] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 194263: clampTo(): do not convert the input to double when dealing with
integers
https://bugs.webkit.org/show_bug.cgi?id=194263

Attachment 361273: Patch

https://bugs.webkit.org/attachment.cgi?id=361273&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 361273
  --> https://bugs.webkit.org/attachment.cgi?id=361273
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361273&action=review

Don’t we also want to test clamping smaller sizes to larger ones to be sure it
does nothing? I also think we want to cover peculiar types, such as "char".

> Source/WTF/ChangeLog:12
> +	   In many case, that was very wasteful. WebKit has many uses of
clampTo() with

Tiny grammar mistake or typo, should be "many cases".

> Source/WTF/ChangeLog:13
> +	   the same type as input/output, or with integer types of different
size/size.

Did you mean size/sign?

> Tools/TestWebKitAPI/Tests/WTF/MathExtras.cpp:29
> +#include <wtf/Compiler.h>

This is not needed. MathExtras.h includes Compiler.h.

> Tools/TestWebKitAPI/Tests/WTF/MathExtras.cpp:220
> +TEST(WTF, clampFloatingPointToFloatingPoint)

There seem to be edge cases missing. To cite one example, super-large doubles
with exponents that can’t be represented as float need to clamp to the largest
non-infinite float.

> Tools/TestWebKitAPI/Tests/WTF/MathExtras.cpp:234
> +   
EXPECT_EQ(clampTo<int32_t>(static_cast<FloatingPointType>(std::numeric_limits<i
nt32_t>::max())), std::numeric_limits<int32_t>::max());

I think we’d want to test that <int32_t>::max() -
<FloatingPointType>::epsilon() clamps to <int32_t>::max() - 1 and other cases
like that. Covering both of the edge cases; the last floating point number that
does *not* clamp to max as well as the first one that does.