Bug 194263 – clampTo(): do not convert the input to double when dealing with integers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

clampTo(): do not convert the input to double when dealing with integers

[Benjamin Poulain]

Created attachment 361143 [details]
Patch

[Benjamin Poulain]

<rdar://problem/47692312>

[Benjamin Poulain]

I'll have to specialize the test for windows.

Visual Studio does not like comparison with Infinity: https://stackoverflow.com/questions/25667254/why-do-i-get-warning-c4756-overflow-in-constant-arithmetic-when-returning-float

[Benjamin Poulain]

Created attachment 361236 [details]
Patch

[Benjamin Poulain]

Created attachment 361273 [details]
Patch

[Darin Adler]

Comment on attachment 361273 [details]
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<int32_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.

[Benjamin Poulain]

Thanks for the review! I'll update the tests and land this week.

[Benjamin Poulain]

Created attachment 361445 [details]
Patch

[EWS Watchlist]

Comment on attachment 361445 [details]
Patch

Attachment 361445 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11072632

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 361473 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Benjamin Poulain]

Created attachment 361492 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 361492 [details]
Patch

Clearing flags on attachment: 361492

Committed r241192: <https://trac.webkit.org/changeset/241192>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Quite happy with the change, but still might want to go back later and add one or two more test cases. I think the interesting one is intmax-1-epsilon, not intmax-1 and not intmax-epsilon, although it’s good to cover those too.

[Benjamin Poulain]

(In reply to Darin Adler from comment #14)
> Quite happy with the change, but still might want to go back later and add
> one or two more test cases. I think the interesting one is intmax-1-epsilon,
> not intmax-1 and not intmax-epsilon, although it’s good to cover those too.

I'll add that.

Can you explain why (intmax-1-epsilon) is more interesting? I don't understand why it is different.

[Darin Adler]

(In reply to Benjamin Poulain from comment #15)
> (In reply to Darin Adler from comment #14)
> > Quite happy with the change, but still might want to go back later and add
> > one or two more test cases. I think the interesting one is intmax-1-epsilon,
> > not intmax-1 and not intmax-epsilon, although it’s good to cover those too.
> 
> I'll add that.
> 
> Can you explain why (intmax-1-epsilon) is more interesting? I don't
> understand why it is different.

I was interested in the edge cases. The last floating point number that converts to intmax-1 and the first one that converts to intmax. So I think I got it wrong, I suppose it's intmax-1 and intmax-1+epsilon that are those two.

[Darin Adler]

Or maybe it should be std::nextafter rather than epsilon that we should use to test. So instead of intmax-1+epsilon it could be written more like std::nextafter(intmax-1, intmax).

[Benjamin Poulain]

(In reply to Darin Adler from comment #17)
> Or maybe it should be std::nextafter rather than epsilon that we should use
> to test. So instead of intmax-1+epsilon it could be written more like
> std::nextafter(intmax-1, intmax).

Ok, I get it now! I'll have a look.