| Summary: | [Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings in TestWebKitAPI | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||
| Component: | Tools / Tests | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | darin, jchaffraix, webkit-bug-importer, zalan | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=204834 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Fujii Hironori
2020-04-06 13:19:52 PDT
Created attachment 395611 [details]
Patch
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? 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? 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. 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. 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. 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. Created attachment 395645 [details]
Patch
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. 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? 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); > ~~~~~~~~~~~~~~~ ~~~~~~~~^~~ Created attachment 395734 [details]
Patch for landing
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. Committed r259704: <https://trac.webkit.org/changeset/259704> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395734 [details]. |