Summary: | error: converting 'false' to pointer type when compiling with gcc-4.6.0 | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | asharif.tools | ||||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, thakis, webkit.review.bot | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Attachments: |
|
Description
asharif.tools
2011-06-06 17:11:27 PDT
Index: Element.cpp =================================================================== --- Element.cpp (revision 87771) +++ Element.cpp (working copy) @@ -1095,7 +1095,7 @@ { // Ref currentStyle in case it would otherwise be deleted when setRenderStyle() is called. RefPtr<RenderStyle> currentStyle(renderStyle()); - bool hasParentStyle = parentNodeForRenderingAndStyle() ? parentNodeForRenderingAndStyle()->renderStyle() : false; + bool hasParentStyle = parentNodeForRenderingAndStyle() ? (parentNodeForRenderingAndStyle()->renderStyle()? true:false) : false; bool hasDirectAdjacentRules = currentStyle && currentStyle->childrenAffectedByDirectAdjacentRules(); bool hasIndirectAdjacentRules = currentStyle && currentStyle->childrenAffectedByForwardPositionalRules(); Seems to fix the warning. Created attachment 103692 [details]
patch to fix some warnings.
I attached a patch to fix the warnings. Can someone review and submit it? Comment on attachment 103692 [details] patch to fix some warnings. View in context: https://bugs.webkit.org/attachment.cgi?id=103692&action=review > Source/WebCore/dom/Element.cpp:1100 > - bool hasParentStyle = parentNodeForRenderingAndStyle() ? parentNodeForRenderingAndStyle()->renderStyle() : false; > + bool hasParentStyle = parentNodeForRenderingAndStyle() ? (bool) parentNodeForRenderingAndStyle()->renderStyle() : false; WebKit usually uses c++ style casts, not c-style casts: static_cast<bool>(...) Comment on attachment 103692 [details] patch to fix some warnings. View in context: https://bugs.webkit.org/attachment.cgi?id=103692&action=review >> Source/WebCore/dom/Element.cpp:1100 >> + bool hasParentStyle = parentNodeForRenderingAndStyle() ? (bool) parentNodeForRenderingAndStyle()->renderStyle() : false; > > WebKit usually uses c++ style casts, not c-style casts: > > static_cast<bool>(...) What do you think between: bool hasParentStyle = parentNodeForRenderingAndStyle() ? (parentNodeForRenderingAndStyle()->renderStyle() ? true : false) : false; and bool hasParentStyle = parentNodeForRenderingAndStyle() ? static_cast<bool>(parentNodeForRenderingAndStyle()->renderStyle()) : false; > bool hasParentStyle = parentNodeForRenderingAndStyle() ? static_cast<bool>(parentNodeForRenderingAndStyle()->renderStyle()) : false;
^^^ this one is preferred.
Created attachment 103702 [details]
Addressed reviewer's comments.
Ping. Can this be reviewed again? You need to set r? on your new patch. Attachment 103702 [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/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
The changelog needs to have a link to the bug right after the title line, see the other changelog entries. Created attachment 103986 [details]
Addressed reviewer's comments about patch ChangeLog formatting.
Ping. Can this be reviewed? You need to remove the "No new tests (OOPS)" line as well. See http://www.webkit.org/coding/contributing.html Created attachment 104120 [details]
Removed the OOPS no new tests line.
Looks good. abarth, can you r+? Comment on attachment 104120 [details] Removed the OOPS no new tests line. Rejecting attachment 104120 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2 Last 500 characters of output: from bug 62168. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 2 Parsed 3 diffs from patch file(s). patching file Source/WebCore/ChangeLog patch: **** malformed patch at line 22: patching file Source/WebCore/dom/Element.cpp patching file Source/WebCore/platform/ScrollAnimatorNone.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 2 Full output: http://queues.webkit.org/results/9405502 Created attachment 104366 [details]
Rebased patch
Attachment 104366 [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/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 104366 [details] Rebased patch r- over stylebot complaint I stumbled over the two missing `return 0`s myself yesterday and I'm fixing them here: https://bugs.webkit.org/show_bug.cgi?id=66480 You can remove them from this patch. Created attachment 104373 [details]
Addressed comments.
Attachment 104373 [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/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 104373 [details] Addressed comments. View in context: https://bugs.webkit.org/attachment.cgi?id=104373&action=review >> Source/WebCore/ChangeLog:1 >> +2011-08-11 Ahmad Sharif <asharif@chromium.org> > > ChangeLog entry has no bug number [changelog/bugnumber] [5] Need to fix this. > Source/WebCore/dom/Element.cpp:1100 > + bool hasParentStyle = parentNodeForRenderingAndStyle() ? static_cast<bool>(parentNodeForRenderingAndStyle()->renderStyle()) : false; I think it would be slightly more elegant to do something like "!!" rather than static_cast<bool>; not really sure. A much clearer way to write it would be to add a hasStyle function to RenderObject* that encapsulates this. Or even just a local helper function called hasStyle. Generally speaking, casts are harder to read than almost any alternative. Created attachment 104530 [details]
Proposed fix.
I haven't fully tested this fix because WebKit doesn't build on my machine (problems with dependencies with --gtk, build failures with --qt) but at least the changed files were compiling fine. Can you send this out to the patch tester and commit for me? Please rebase if necessary.
Attachment 104530 [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/dom/ContainerNode.cpp:766: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 104533 [details]
Fixed formatting issues.
Attachment 104533 [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/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 104545 [details]
Added bug link.
Comment on attachment 104545 [details] Added bug link. Attachment 104545 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9432439 New failing tests: canvas/philip/tests/2d.text-custom-font-load-crash.html http/tests/cache/subresource-expiration-1.html http/tests/inspector/console-websocket-error.html css3/selectors3/xml/css3-modsel-25.xml http/tests/inspector/console-cd.html http/tests/inspector/console-cd-completions.html http/tests/inspector/resource-har-headers.html http/tests/inspector-enabled/console-clear-arguments-on-frame-remove.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/cache/subresource-expiration-2.html http/tests/inspector/console-xhr-logging.html animations/body-removal-crash.html http/tests/inspector/network-preflight-options.html http/tests/cache/subresource-multiple-instances.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/console-resource-errors.html http/tests/inspector/resource-parameters.html http/tests/inspector-enabled/console-log-before-frame-navigation.html animations/dynamic-stylesheet-loading.html http/tests/inspector/change-iframe-src.html Comment on attachment 104545 [details] Added bug link. View in context: https://bugs.webkit.org/attachment.cgi?id=104545&action=review > Source/WebCore/dom/ContainerNode.cpp:764 > +bool ContainerNode::hasStyle() const Thanks for doing what I asked. Style-wise this is way better. Since this code is quite hot, I think we probably want to make sure this is inlined. It’s slightly tricky to do that because we can’t put it in ContainerNode.h. Sorry, this is getting unpleasantly complicated. I guess we can try with it not inlined, but I do fear this will make things slower. > Source/WebCore/dom/ContainerNode.cpp:766 > + if (renderer()->style()) What guarantees the renderer is non-zero? I think we need to check that too. Comment on attachment 104545 [details]
Added bug link.
Given these new issues that arising, I think we should probably just go back to the static_cast<bool> version. Sorry for the churn.
Created attachment 104573 [details]
Used static_cast<bool> to fix compiler warning.
Just rebased this and it should work I think.
Ping. Can someone rebase and submit? Created attachment 104883 [details]
rebased patch
Comment on attachment 104883 [details] rebased patch Clearing flags on attachment: 104883 Committed r93631: <http://trac.webkit.org/changeset/93631> All reviewed patches have been landed. Closing bug. |