Bug 101458 - Fix compile warning [-Wsign-compare]
Summary: Fix compile warning [-Wsign-compare]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: KyungTae Kim
URL:
Keywords:
Depends on:
Blocks: 101761
  Show dependency treegraph
 
Reported: 2012-11-07 05:46 PST by Kangil Han
Modified: 2012-11-15 21:48 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.51 KB, patch)
2012-11-07 05:50 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (2.06 KB, patch)
2012-11-07 17:05 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (2.03 KB, patch)
2012-11-07 18:19 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (1.64 KB, patch)
2012-11-13 17:09 PST, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (1.65 KB, patch)
2012-11-13 20:54 PST, KyungTae Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kangil Han 2012-11-07 05:46:46 PST
WebKit/Source/WebCore/platform/posix/SharedBufferPOSIX.cpp:55:33: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
Comment 1 Kangil Han 2012-11-07 05:50:00 PST
Created attachment 172777 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-11-07 09:45:45 PST
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.
Comment 3 Kangil Han 2012-11-07 17:05:15 PST
Created attachment 172894 [details]
Patch
Comment 4 Kangil Han 2012-11-07 17:05:38 PST
(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. :)
Comment 5 Alexey Proskuryakov 2012-11-07 17:35:24 PST
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).
Comment 6 Kangil Han 2012-11-07 18:19:45 PST
Created attachment 172906 [details]
Patch
Comment 7 Rafael Brandao 2012-11-07 20:29:54 PST
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.
Comment 8 KyungTae Kim 2012-11-13 01:14:25 PST
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?
Comment 9 Kangil Han 2012-11-13 02:53:28 PST
Reset assignee and obsolete all my patches for next patch candidate.
Good luck! :)
Comment 10 KyungTae Kim 2012-11-13 17:09:07 PST
Created attachment 174031 [details]
Patch
Comment 11 Alexey Proskuryakov 2012-11-13 19:40:14 PST
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).
Comment 12 KyungTae Kim 2012-11-13 20:54:35 PST
Created attachment 174063 [details]
Patch
Comment 13 Alexey Proskuryakov 2012-11-13 21:36:10 PST
Comment on attachment 174063 [details]
Patch

I guess a runtime check for <0 is fine as well.
Comment 14 Kangil Han 2012-11-13 21:56:51 PST
Nice job, Kyungtae! :-)
Comment 15 WebKit Review Bot 2012-11-13 22:15:44 PST
Comment on attachment 174063 [details]
Patch

Clearing flags on attachment: 174063

Committed r134544: <http://trac.webkit.org/changeset/134544>
Comment 16 WebKit Review Bot 2012-11-13 22:15:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Darin Adler 2012-11-15 20:55:47 PST
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.
Comment 18 KyungTae Kim 2012-11-15 21:13:04 PST
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.
Comment 19 Alexey Proskuryakov 2012-11-15 21:48:44 PST
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.