New builds of clang warn about boolean checks for the |this| pointer being undefined behavior: Source/WebCore/rendering/RenderObject.cpp:1465:10: error: 'this' pointer cannot be null in wel l-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion] if (!this) { ~^~~~ Source/WebCore/rendering/RenderObject.cpp:1584:10: error: 'this' pointer cannot be null in wel l-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion] if (!this) ~^~~~ 2 errors generated.
BTW, changing the check from "!this" to "this == nullptr" triggers a different warning: Source/WebCore/rendering/RenderObject.cpp:1460:9: error: 'this' pointer cannot be null in well-defined C++ code; comparison may be assumed to always evaluate to false [-Werror,-Wtautological-undefined-compare] if (this == nullptr) { ^~~~ ~~~~~~~ Source/WebCore/rendering/RenderObject.cpp:1579:9: error: 'this' pointer cannot be null in well-defined C++ code; comparison may be assumed to always evaluate to false [-Werror,-Wtautological-undefined-compare] if (this == nullptr) ^~~~ ~~~~~~~ 2 errors generated.
And adding #pragma statements to ignore the warning causes unknown warnings in older clang: /Volumes/Data/WebKit.git/Source/WebCore/rendering/RenderObject.cpp:1457:34: error: unknown warning group '-Wundefined-bool-conversion', ignored [-Werror,-Wunknown-pragmas] #pragma clang diagnostic ignored "-Wundefined-bool-conversion" ^ /Volumes/Data/WebKit.git/Source/WebCore/rendering/RenderObject.cpp:1576:34: error: unknown warning group '-Wundefined-bool-conversion', ignored [-Werror,-Wunknown-pragmas] #pragma clang diagnostic ignored "-Wundefined-bool-conversion" ^ 2 errors generated. I guess I either need to ignore the unknown warnings in older clangs, or use __has_feature() (assuming that works for compiler flags).
Created attachment 237735 [details] Patch v1
Comment on attachment 237735 [details] Patch v1 Clearing flags on attachment: 237735 Committed r173357: <http://trac.webkit.org/changeset/173357>
All reviewed patches have been landed. Closing bug.
Comment on attachment 237735 [details] Patch v1 I think we should just remove the null check. We should never get here when |this| is null. It's debug-only code anyway.
(In reply to comment #6) > (From update of attachment 237735 [details]) > I think we should just remove the null check. We should never get here when |this| is null. It's debug-only code anyway. In an lldb session, it's possible to call showRenderTree() on an invalid object.
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 237735 [details] [details]) > > I think we should just remove the null check. We should never get here when |this| is null. It's debug-only code anyway. > In an lldb session, it's possible to call showRenderTree() on an invalid object. Yes, but if you try it you’ll see there’s no disastrous result. I think we can consider taking out the null checks. Or we could make these non-member functions. I think showRenderTree(x) is just as easy to type in the debugger.
I think we should optimize these various dumping functions for easy typing in lldb. For example, I think they should be called showTree, not showRenderTree, and I think that it’s easier to type showTree(x) than x->showTree().
(In reply to comment #9) > I think we should optimize these various dumping functions for easy typing in lldb. For example, I think they should be called showTree, not showRenderTree, and I think that it’s easier to type showTree(x) than x->showTree(). Currently we've got the following non-member show*Tree() functions. void showNodeTree(const WebCore::RenderObject*); void showLineTree(const WebCore::RenderObject*); void showRenderTree(const WebCore::RenderObject*); and the member helpers: void showNodeTreeForThis() const; void showRenderTreeForThis() const; void showLineTreeForThis() const; so you can type showRenderTree(x) or showRenderTreeForThis() and they output the same when x == this.
(In reply to comment #10) > Currently we've got the following non-member show*Tree() functions. > void showNodeTree(const WebCore::RenderObject*); > void showLineTree(const WebCore::RenderObject*); > void showRenderTree(const WebCore::RenderObject*); > > and the member helpers: > void showNodeTreeForThis() const; > void showRenderTreeForThis() const; > void showLineTreeForThis() const; > > so you can type showRenderTree(x) or showRenderTreeForThis() and they output the same when x == this. OK. Lets take the null checks out of the member helpers and put them only in the free functions. Unless that creates any real inconvenience for someone.
Reopening to attach new patch.
Created attachment 260444 [details] Patch
Comment on attachment 260444 [details] Patch r=me
(In reply to comment #13) > Created attachment 260444 [details] > Patch This is a follow up patch based on the discussion above. (tl;dr: there'd be no disastrous result, if this check was removed)
Comment on attachment 260444 [details] Patch Rejecting attachment 260444 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 260444, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit Committing to http://svn.webkit.org/repository/webkit/trunk ... Authentication realm: <http://svn.webkit.org:80> Mac OS Forge Password for 'commit-queue@webkit.org': Authentication realm: <http://svn.webkit.org:80> Mac OS Forge Username: error: git-svn died of signal 11 Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Full output: http://webkit-queues.webkit.org/results/134415
Created attachment 260482 [details] Patch
Comment on attachment 260482 [details] Patch Rejecting attachment 260482 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 260482, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit Committing to http://svn.webkit.org/repository/webkit/trunk ... Authentication realm: <http://svn.webkit.org:80> Mac OS Forge Password for 'commit-queue@webkit.org': Authentication realm: <http://svn.webkit.org:80> Mac OS Forge Username: error: git-svn died of signal 11 Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Full output: http://webkit-queues.webkit.org/results/135259
Comment on attachment 260482 [details] Patch Clearing flags on attachment: 260482 Committed r189310: <http://trac.webkit.org/changeset/189310>