"if (!this || !&other)" in QuotesData::operator==() doesn't generate expected code when being compiled with RVCT. This can cause crashes
Created attachment 103403 [details] the patch
Attachment 103403 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/style/QuotesData.h:37: The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 103403 [details] the patch Attachment 103403 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9340314
Comment on attachment 103403 [details] the patch Attachment 103403 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9338347
Comment on attachment 103403 [details] the patch Attachment 103403 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9332544
Comment on attachment 103403 [details] the patch Attachment 103403 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9340320
Comment on attachment 103403 [details] the patch Attachment 103403 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9338352
Comment on attachment 103403 [details] the patch Attachment 103403 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9337415
Created attachment 103484 [details] updated
Attachment 103484 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/style/QuotesData.h:37: The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 103489 [details] Fix style issue No more "a"
Comment on attachment 103489 [details] Fix style issue View in context: https://bugs.webkit.org/attachment.cgi?id=103489&action=review > Source/WebCore/rendering/style/QuotesData.h:37 > - bool operator==(const QuotesData&) const; > void operator delete(void* p) { delete[] static_cast<char*>(p); } > + static bool equal(const QuotesData*, const QuotesData*); Should we add a comment about avoiding "==" so no one tries to get it back in?
(In reply to comment #12) > (From update of attachment 103489 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103489&action=review > > > Source/WebCore/rendering/style/QuotesData.h:37 > > - bool operator==(const QuotesData&) const; > > void operator delete(void* p) { delete[] static_cast<char*>(p); } > > + static bool equal(const QuotesData*, const QuotesData*); > > Should we add a comment about avoiding "==" so no one tries to get it back in? operator== is not a problem. The problem is the old operator== is bad written. It checks "this" pointer and the address of a reference (QuotesData&), and expects to work with null "this" and a reference deferencing a null pointer. If someone in the future wants to write a normal operator==, and doesn't use operator== on null pointers, it should be fine.
> The problem is the old operator== is bad written Are you sure that the blame is pointed correctly here? Seems like a bug in this particular compiler to me.
(In reply to comment #14) > > The problem is the old operator== is bad written > > Are you sure that the blame is pointed correctly here? Seems like a bug in this particular compiler to me. This was discussed @ https://bugs.webkit.org/show_bug.cgi?id=36901 from [Daniel Bates]: By 8.3.2 (5) of the C++ standard <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf>: A reference shall be initialized to refer to a valid object or function. [ Note: in particular, a null reference cannot exist in a well-defined program, because the only way to create such a reference would be to bind it to the “object” obtained by dereferencing a null pointer, which causes undefined behavior....] RVCT uses a strict interpretation of the standard (compared to GCC) and assumes a reference must always be initialized with a valid object. Hence it optimizes away !&other. Another one is "!this". Although RVCT has a compiler option "--allow_null_this", we should still avoid writing such code.
Comment on attachment 103489 [details] Fix style issue Thanks Alexey!
Comment on attachment 103489 [details] Fix style issue Clearing flags on attachment: 103489 Committed r92871: <http://trac.webkit.org/changeset/92871>
All reviewed patches have been landed. Closing bug.