clampTo(): do not convert the input to double when dealing with integers
Created attachment 361143 [details] Patch
<rdar://problem/47692312>
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
Created attachment 361236 [details] Patch
Created attachment 361273 [details] Patch
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.
Thanks for the review! I'll update the tests and land this week.
Created attachment 361445 [details] Patch
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
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
Created attachment 361492 [details] Patch
Comment on attachment 361492 [details] Patch Clearing flags on attachment: 361492 Committed r241192: <https://trac.webkit.org/changeset/241192>
All reviewed patches have been landed. Closing bug.
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.
(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.
(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.
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).
(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.