[webkit-reviews] review denied: [Bug 210067] [Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings in TestWebKitAPI : [Attachment 395611] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 210067: [Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings
in TestWebKitAPI
https://bugs.webkit.org/show_bug.cgi?id=210067

Attachment 395611: Patch

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




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

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

> Tools/TestWebKitAPI/Tests/WTF/MathExtras.cpp:96
> +    float maxInt = maxPlusOne<int>;

Local variable name is not good now, since it's past the max int. Not clear if
this is OK for what we are testing. We want to test values right around the
border of what fits in an int to see they are handled correctly. It’s not OK to
just test std::numeric_limits<int>::max() + 1, we also want to test smaller
values.

> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:774
> -    maxIntRect.shiftMaxXEdgeTo(INT_MAX);
> -    maxIntRect.shiftMaxYEdgeTo(INT_MAX);
> +    maxIntRect.shiftMaxXEdgeTo(maxPlusOne<int>);
> +    maxIntRect.shiftMaxYEdgeTo(maxPlusOne<int>);

This is not what we are trying to test. We are trying to test INT_MAX, not
INT_MAX + 1.

> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:781
> -    EXPECT_EQ(INT_MAX, enclosed2.width());
> -    EXPECT_EQ(INT_MAX, enclosed2.height());
> +    EXPECT_EQ(maxPlusOne<int>, enclosed2.width());
> +    EXPECT_EQ(maxPlusOne<int>, enclosed2.height());

This looks wrong. Why would a floating point number be involved in checking the
resulting integer value?