Bug 62168

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 Flags
patch to fix some warnings.
none
Addressed reviewer's comments.
thakis: review-
Addressed reviewer's comments about patch ChangeLog formatting.
thakis: review-
Removed the OOPS no new tests line.
abarth: review+, webkit.review.bot: commit-queue-
Rebased patch
thakis: review-, thakis: commit-queue-
Addressed comments.
darin: review+, darin: commit-queue-
Proposed fix.
none
Fixed formatting issues.
none
Added bug link.
webkit.review.bot: commit-queue-
Used static_cast<bool> to fix compiler warning.
none
rebased patch none

Description asharif.tools 2011-06-06 17:11:27 PDT
third_party/WebKit/Source/WebCore/dom/Element.cpp: In member function 'virtual void WebCore::Element::recalcStyle(WebCore::Node::StyleChange)':
third_party/WebKit/Source/WebCore/dom/Element.cpp:1098:112: error: converting 'false' to pointer type 'WebCore::RenderStyle*' [-Werror=conversion-null]

This can easily be fixed in the source (1 line change).
Comment 1 asharif.tools 2011-06-06 17:13:08 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.
Comment 2 asharif.tools 2011-08-11 15:51:27 PDT
Created attachment 103692 [details]
patch to fix some warnings.
Comment 3 asharif.tools 2011-08-11 15:52:21 PDT
I attached a patch to fix the warnings. Can someone review and submit it?
Comment 4 Adam Barth 2011-08-11 16:22:20 PDT
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 5 asharif.tools 2011-08-11 16:33:07 PDT
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;
Comment 6 Adam Barth 2011-08-11 16:39:26 PDT
> bool hasParentStyle = parentNodeForRenderingAndStyle() ? static_cast<bool>(parentNodeForRenderingAndStyle()->renderStyle()) : false;

^^^ this one is preferred.
Comment 7 asharif.tools 2011-08-11 16:42:20 PDT
Created attachment 103702 [details]
Addressed reviewer's comments.
Comment 8 asharif.tools 2011-08-15 12:28:08 PDT
Ping. Can this be reviewed again?
Comment 9 Nico Weber 2011-08-15 12:30:16 PDT
You need to set r? on your new patch.
Comment 10 WebKit Review Bot 2011-08-15 12:33:29 PDT
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.
Comment 11 Nico Weber 2011-08-15 15:30:56 PDT
The changelog needs to have a link to the bug right after the title line, see the other changelog entries.
Comment 12 asharif.tools 2011-08-15 17:44:56 PDT
Created attachment 103986 [details]
Addressed reviewer's comments about patch ChangeLog formatting.
Comment 13 asharif.tools 2011-08-16 14:01:00 PDT
Ping. Can this be reviewed?
Comment 14 Nico Weber 2011-08-16 14:03:28 PDT
You need to remove the "No new tests (OOPS)" line as well. See http://www.webkit.org/coding/contributing.html
Comment 15 asharif.tools 2011-08-16 16:49:16 PDT
Created attachment 104120 [details]
Removed the OOPS no new tests line.
Comment 16 Nico Weber 2011-08-16 16:50:01 PDT
Looks good. abarth, can you r+?
Comment 17 WebKit Review Bot 2011-08-16 19:05:56 PDT
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
Comment 18 asharif.tools 2011-08-18 11:06:03 PDT
Created attachment 104366 [details]
Rebased patch
Comment 19 WebKit Review Bot 2011-08-18 11:13:20 PDT
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 20 Nico Weber 2011-08-18 11:31:21 PDT
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.
Comment 21 asharif.tools 2011-08-18 11:44:12 PDT
Created attachment 104373 [details]
Addressed comments.
Comment 22 WebKit Review Bot 2011-08-18 11:46:27 PDT
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 23 Darin Adler 2011-08-18 11:51:45 PDT
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.
Comment 24 asharif.tools 2011-08-19 11:19:13 PDT
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.
Comment 25 WebKit Review Bot 2011-08-19 11:21:34 PDT
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.
Comment 26 asharif.tools 2011-08-19 11:31:39 PDT
Created attachment 104533 [details]
Fixed formatting issues.
Comment 27 WebKit Review Bot 2011-08-19 11:35:11 PDT
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.
Comment 28 asharif.tools 2011-08-19 12:21:43 PDT
Created attachment 104545 [details]
Added bug link.
Comment 29 WebKit Review Bot 2011-08-19 13:17:20 PDT
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 30 Darin Adler 2011-08-19 13:27:12 PDT
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 31 Darin Adler 2011-08-19 13:27:44 PDT
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.
Comment 32 asharif.tools 2011-08-19 14:50:47 PDT
Created attachment 104573 [details]
Used static_cast<bool> to fix compiler warning.

Just rebased this and it should work I think.
Comment 33 asharif.tools 2011-08-22 10:36:34 PDT
Ping. Can someone rebase and submit?
Comment 34 asharif.tools 2011-08-23 12:05:57 PDT
Created attachment 104883 [details]
rebased patch
Comment 35 WebKit Review Bot 2011-08-23 13:10:58 PDT
Comment on attachment 104883 [details]
rebased patch

Clearing flags on attachment: 104883

Committed r93631: <http://trac.webkit.org/changeset/93631>
Comment 36 WebKit Review Bot 2011-08-23 13:11:05 PDT
All reviewed patches have been landed.  Closing bug.