Bug 62168 – error: converting 'false' to pointer type when compiling with gcc-4.6.0

• Component: WebKit Misc.
• Status: RESOLVED FIXED

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).

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?

[Adam Barth]

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;

[Adam Barth]

> 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?

[Nico Weber]

You need to set r? on your new patch.

[WebKit Review Bot]

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.

[Nico Weber]

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?

[Nico Weber]

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.

[Nico Weber]

Looks good. abarth, can you r+?

[WebKit Review Bot]

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

[WebKit Review Bot]

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.

[Nico Weber]

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.

[WebKit Review Bot]

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.

[Darin Adler]

Comment on attachment 104373 [details]
Addressed comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=104373&action=review

>> Source/WebCore/ChangeLog:1
>> [phone]  Ahmad Sharif  <[e-mail]>
> 
> 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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Darin Adler]

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.

[Darin Adler]

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

[WebKit Review Bot]

Comment on attachment 104883 [details]
rebased patch

Clearing flags on attachment: 104883

Committed r93631: <http://trac.webkit.org/changeset/93631>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.