WebKit/Source/WebCore/platform/posix/SharedBufferPOSIX.cpp:55:33: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
Created attachment 172777 [details] Patch
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.
Created attachment 172894 [details] Patch
(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 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).
Created attachment 172906 [details] Patch
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 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?
Reset assignee and obsolete all my patches for next patch candidate. Good luck! :)
Created attachment 174031 [details] Patch
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).
Created attachment 174063 [details] Patch
Comment on attachment 174063 [details] Patch I guess a runtime check for <0 is fine as well.
Nice job, Kyungtae! :-)
Comment on attachment 174063 [details] Patch Clearing flags on attachment: 174063 Committed r134544: <http://trac.webkit.org/changeset/134544>
All reviewed patches have been landed. Closing bug.
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.
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 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.