Summary: | Crash in QuotesData::operator== when compiled with RVCT | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yong Li <yong.li.webkit> | ||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, dglazkov, gustavo, jbedard, webkit.review.bot, xan.lopez | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Yong Li
2011-08-09 14:35:53 PDT
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. |