Bug 101458 – Fix compile warning [-Wsign-compare]

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kangil Han]

WebKit/Source/WebCore/platform/posix/SharedBufferPOSIX.cpp:55:33: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

[Kangil Han]

Created attachment 172777 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 172777 [details]
Patch

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

This change at least needs a detailed explanation, but is likely incorrect.

> Source/WebCore/ChangeLog:8
> +        Fix compile warning by using early return when file size is less than or equal to zero.

How can file size be less than zero?

> Source/WebCore/platform/posix/SharedBufferPOSIX.cpp:54
> -    size_t bytesToRead = fileStat.st_size;
> -    if (bytesToRead != fileStat.st_size) {
> +    if (fileStat.st_size <= 0) {

I'm not exactly sure what this check was meant for. It looks like it's meant to take care of platforms where st_size type is wider than size_t, so such conversion is lossy. Proposed code doesn't handle that.

[Kangil Han]

Created attachment 172894 [details]
Patch

[Kangil Han]

(In reply to comment #2)
> 
> I'm not exactly sure what this check was meant for. It looks like it's meant to take care of platforms where st_size type is wider than size_t, so such conversion is lossy. Proposed code doesn't handle that.

Done. :)

[Alexey Proskuryakov]

Comment on attachment 172894 [details]
Patch

As discussed on IRC, this doesn't seem right either, because casting numeric_limits<size_t>::max() to off_t does not necessarily work (and in fact, it does not work on your platform, where they have the same width, so the result becomes negative).

[Kangil Han]

Created attachment 172906 [details]
Patch

[Rafael Brandao]

Comment on attachment 172906 [details]
Patch

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

> Source/WebCore/platform/posix/SharedBufferPOSIX.cpp:61
> +    ptrdiff_t conversionedFileSize = fileStat.st_size;

After you check it's not negative, then I think the only way their values could be different is if size_of(size_t) != size_of(off_t), right? Not sure if this would be a better check though, just shouting an idea.

[KyungTae Kim]

Comment on attachment 172906 [details]
Patch

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

> Source/WebCore/platform/posix/SharedBufferPOSIX.cpp:62
> +    if (conversionedFileSize != fileStat.st_size) {

How about include "fileStat.st_size < 0" check in here to avoid duplication?

[Kangil Han]

Reset assignee and obsolete all my patches for next patch candidate.
Good luck! :)

[KyungTae Kim]

Created attachment 174031 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 174031 [details]
Patch

I think that this is the right fix. You can consider adding an assertion that st_size is non-negative.

Please fix coding style though - we prefer C++ style casts (static_cast).

[KyungTae Kim]

Created attachment 174063 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 174063 [details]
Patch

I guess a runtime check for <0 is fine as well.

[Kangil Han]

Nice job, Kyungtae! :-)

[WebKit Review Bot]

Comment on attachment 174063 [details]
Patch

Clearing flags on attachment: 174063

Committed r134544: <http://trac.webkit.org/changeset/134544>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 174063 [details]
Patch

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

> Source/WebCore/platform/posix/SharedBufferPOSIX.cpp:55
> +    if (fileStat.st_size < 0 || bytesToRead != static_cast<unsigned long long>(fileStat.st_size)) {

What’s the point of the size < 0 check, again? Converting a signed integer to unsigned long long will create a giant size that won’t possibly equal bytesToRead, so I see no reason to need that.

[KyungTae Kim]

Darin Adler :
I agree with you on that. That's why my 1st patch was not included < 0 check.

But the 2nd patch is more clear for human read, I think.

If you think 1st patch is better, I'll fix it to that and add a comment that explains the < 0 case.

[Alexey Proskuryakov]

Comment on attachment 174063 [details]
Patch

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

>> Source/WebCore/platform/posix/SharedBufferPOSIX.cpp:55
>> +    if (fileStat.st_size < 0 || bytesToRead != static_cast<unsigned long long>(fileStat.st_size)) {
> 
> What’s the point of the size < 0 check, again? Converting a signed integer to unsigned long long will create a giant size that won’t possibly equal bytesToRead, so I see no reason to need that.

Can't they be equal if size_t is the same size as unsigned long long?

I guess this combines a sanity check with a loss of precision check.