Bug 210067 – [Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings in TestWebKitAPI

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Fujii Hironori]

[Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings in TestWebKitAPI

This is a sub-task of Bug 204834.

Clang 10 reports compilation warnings in TestWebKitAPI:

> [2108/5311] Building CXX object Tools\TestWebKitAPI\CMakeFiles\TestWTFLib.dir\Tests\WTF\MathExtras.cpp.obj
> ..\..\Tools\TestWebKitAPI\Tests\WTF\MathExtras.cpp(96,20): warning: implicit conversion from 'int' to 'float' changes value from 2147483647 to 2147483648 [-Wimplicit-int-float-conversion]
>     float maxInt = std::numeric_limits<int>::max();
>           ~~~~~~   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.

> [3798/5311] Building CXX object Tools\TestWebKitAPI\CMakeFiles\TestWebCoreLib.dir\Tests\WebCore\FloatRect.cpp.obj
> ..\..\Tools\TestWebKitAPI\Tests\WebCore\FloatRect.cpp(773,32): warning: implicit conversion from 'int' to 'float' changes value from 2147483647 to 2147483648 [-Wimplicit-int-float-conversion]
>     maxIntRect.shiftMaxXEdgeTo(INT_MAX);
>                ~~~~~~~~~~~~~~~ ^~~~~~~
> C:\PROGRA~1\LLVM\lib\clang\10.0.0\include\limits.h(46,19): note: expanded from macro 'INT_MAX'
> #define INT_MAX   __INT_MAX__
>                   ^~~~~~~~~~~
> <built-in>(81,21): note: expanded from here
> #define __INT_MAX__ 2147483647
>                     ^~~~~~~~~~
> ..\..\Tools\TestWebKitAPI\Tests\WebCore\FloatRect.cpp(774,32): warning: implicit conversion from 'int' to 'float' changes value from 2147483647 to 2147483648 [-Wimplicit-int-float-conversion]
>     maxIntRect.shiftMaxYEdgeTo(INT_MAX);
>                ~~~~~~~~~~~~~~~ ^~~~~~~
> C:\PROGRA~1\LLVM\lib\clang\10.0.0\include\limits.h(46,19): note: expanded from macro 'INT_MAX'
> #define INT_MAX   __INT_MAX__
>                   ^~~~~~~~~~~
> <built-in>(81,21): note: expanded from here
> #define __INT_MAX__ 2147483647
>                     ^~~~~~~~~~
> 2 warnings generated.


See also: r259537 (for WTF) and r259588 (for JavaScriptCore)

[Fujii Hironori]

Created attachment 395611 [details]
Patch

[Darin Adler]

Comment on attachment 395611 [details]
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?

[Fujii Hironori]

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

Agreed. I will test INT_MAX - 1 and INT_MAX + 1.

>> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:774
>> +    maxIntRect.shiftMaxYEdgeTo(maxPlusOne<int>);
> 
> This is not what we are trying to test. We are trying to test INT_MAX, not INT_MAX + 1.

float can't keep the precision of INT_MAX. As the compilation warning explains, INT_MAX is converted a float of INT_MAX+1 in ordinary architectures.
The implementation of FloatRect doesn't use 'int' at all.  I don't think it makes sense to test both INT_MAX-1 and INT_MAX+1 here.
What do you think?

>> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:781
>> +    EXPECT_EQ(maxPlusOne<int>, enclosed2.height());
> 
> This looks wrong. Why would a floating point number be involved in checking the resulting integer value?

They (maxPlusOne<int>, enclosed2.width() and enclosed2.height()) are all float type. What do you mean?

[Darin Adler]

Comment on attachment 395611 [details]
Patch

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

>>> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:774
>>> +    maxIntRect.shiftMaxYEdgeTo(maxPlusOne<int>);
>> 
>> This is not what we are trying to test. We are trying to test INT_MAX, not INT_MAX + 1.
> 
> float can't keep the precision of INT_MAX. As the compilation warning explains, INT_MAX is converted a float of INT_MAX+1 in ordinary architectures.
> The implementation of FloatRect doesn't use 'int' at all.  I don't think it makes sense to test both INT_MAX-1 and INT_MAX+1 here.
> What do you think?

We should test float values closest to INT_MAX, just below, ==, just above. Not sure how to generate those values.

>>> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:781
>>> +    EXPECT_EQ(maxPlusOne<int>, enclosed2.height());
>> 
>> This looks wrong. Why would a floating point number be involved in checking the resulting integer value?
> 
> They (maxPlusOne<int>, enclosed2.width() and enclosed2.height()) are all float type. What do you mean?

No, they are not. enclosed2 is an IntRect. WebCore::enclosingIntRect returns an IntRect.

[Fujii Hironori]

Comment on attachment 395611 [details]
Patch

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

>>>> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:781
>>>> +    EXPECT_EQ(maxPlusOne<int>, enclosed2.height());
>>> 
>>> This looks wrong. Why would a floating point number be involved in checking the resulting integer value?
>> 
>> They (maxPlusOne<int>, enclosed2.width() and enclosed2.height()) are all float type. What do you mean?
> 
> No, they are not. enclosed2 is an IntRect. WebCore::enclosingIntRect returns an IntRect.

Oh my bad. You are right.

[Fujii Hironori]

Comment on attachment 395611 [details]
Patch

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

>>>> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:774
>>>> +    maxIntRect.shiftMaxYEdgeTo(maxPlusOne<int>);
>>> 
>>> This is not what we are trying to test. We are trying to test INT_MAX, not INT_MAX + 1.
>> 
>> float can't keep the precision of INT_MAX. As the compilation warning explains, INT_MAX is converted a float of INT_MAX+1 in ordinary architectures.
>> The implementation of FloatRect doesn't use 'int' at all.  I don't think it makes sense to test both INT_MAX-1 and INT_MAX+1 here.
>> What do you think?
> 
> We should test float values closest to INT_MAX, just below, ==, just above. Not sure how to generate those values.

Yeah, agreed. I was wrong. They are reasonable tests for enclosingIntRect.

[Fujii Hironori]

Comment on attachment 395611 [details]
Patch

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

>>>>> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:774
>>>>> +    maxIntRect.shiftMaxYEdgeTo(maxPlusOne<int>);
>>>> 
>>>> This is not what we are trying to test. We are trying to test INT_MAX, not INT_MAX + 1.
>>> 
>>> float can't keep the precision of INT_MAX. As the compilation warning explains, INT_MAX is converted a float of INT_MAX+1 in ordinary architectures.
>>> The implementation of FloatRect doesn't use 'int' at all.  I don't think it makes sense to test both INT_MAX-1 and INT_MAX+1 here.
>>> What do you think?
>> 
>> We should test float values closest to INT_MAX, just below, ==, just above. Not sure how to generate those values.
> 
> Yeah, agreed. I was wrong. They are reasonable tests for enclosingIntRect.

Umm, this is not simple as I thought.
This is a test case of combining shiftMaxXEdgeTo and enclosingIntRect.
It's enough to test int overflow by doing shiftMaxXEdgeTo(0) because x is INT_MIN.
I can't find out a good way to test non-overflow edge case for them.

[Fujii Hironori]

Created attachment 395645 [details]
Patch

[Darin Adler]

Comment on attachment 395645 [details]
Patch

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

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

Seems like we should use some positive number larger than 0 but smaller than INT_MAX instead.

[Fujii Hironori]

Comment on attachment 395645 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:774
>> +    maxIntRect.shiftMaxYEdgeTo(0);
> 
> Seems like we should use some positive number larger than 0 but smaller than INT_MAX instead.

Why? For signed integer representation other than two’s complement?
I will replace 0 with "INT_MAX / 2". Sounds good?

[Fujii Hironori]

Comment on attachment 395645 [details]
Patch

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

>>> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:774
>>> +    maxIntRect.shiftMaxYEdgeTo(0);
>> 
>> Seems like we should use some positive number larger than 0 but smaller than INT_MAX instead.
> 
> Why? For signed integer representation other than two’s complement?
> I will replace 0 with "INT_MAX / 2". Sounds good?

Oops. It's not good.

> ..\..\Tools\TestWebKitAPI\Tests\WebCore\FloatRect.cpp(773,40): warning: implicit conversion from 'int' to 'float' changes value from 1073741823 to 1073741824 [-Wimplicit-int-float-conversion]
>     maxIntRect.shiftMaxXEdgeTo(INT_MAX / 2);
>                ~~~~~~~~~~~~~~~ ~~~~~~~~^~~

[Fujii Hironori]

Created attachment 395734 [details]
Patch for landing

[Darin Adler]

Comment on attachment 395645 [details]
Patch

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

>>>> Tools/TestWebKitAPI/Tests/WebCore/FloatRect.cpp:774
>>>> +    maxIntRect.shiftMaxYEdgeTo(0);
>>> 
>>> Seems like we should use some positive number larger than 0 but smaller than INT_MAX instead.
>> 
>> Why? For signed integer representation other than two’s complement?
>> I will replace 0 with "INT_MAX / 2". Sounds good?
> 
> Oops. It's not good.

I was just confused. I thought that MAX was the one with larger magnitude, not MIN. So 0 was good enough. But I think 30 is even better.

[EWS]

Committed r259704: <https://trac.webkit.org/changeset/259704>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395734 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61432578>