1073707 - Build warning (treated as error in warnings-as-errors builds): parser/html/nsHtml5TreeBuilder.cpp:3187:9: error: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion]

Status: RESOLVED DUPLICATE of bug 1109311

(Core :: DOM: HTML Parser, defect)

Description

[Daniel Holbert [:dholbert]]

Building with the default clang on Ubuntu 14.10 final beta, I get the following build warning, treated as an error in my --enable-warnings-as-errors build:
{
parser/html/nsHtml5TreeBuilder.cpp:3187:9: error: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion]
     if (this) {
     ~~  ^~~~

parser/html/nsHtml5TreeBuilder.cpp:3193:7: error: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion]
   if (this) {
   ~~  ^~~~
}


clang++ --version says: "Ubuntu clang version 3.5.0-1ubuntu1 (tags/RELEASE_350/final) (based on LLVM 3.5.0)"
clang package version: 3.5-23ubuntu1


hsivonen, do you know why we null-check 'this' here? (Is clang wrong to warn about this?)

Comment 1

[Henri Sivonen (:hsivonen)]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> hsivonen, do you know why we null-check 'this' here?

In the code this is translated from, there is a variable that allows for more modularity. In the translation for Gecko, that variable is flattened to "this". So the null-check makes sense in the source of the translation. In the translation result, the check is useless. It is also harmless, which is why I didn't take the time to flatten the check away.

> (Is clang wrong to warn about this?)

No, but there's no real danger here.

Do we have a mechanism to ask clang not to whine about this for this compilation unit? (If not, I suppose I could make the Java-to-C++ translator flatten out the null-check.)

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Henri Sivonen (:hsivonen) from comment #1)
> Do we have a mechanism to ask clang not to whine about this for this
> compilation unit?

Yeah. It looks like we can add:
#pragma clang diagnostic ignored -Wundefined-bool-conversion
...or something like that, based on an MXR search:
 http://mxr.mozilla.org/mozilla-central/search?string=%23pragma%20clang%20diagnostic%20ignored

(I've never used that exact invocation before, but I've had success with similar #pragmas for GCC & MSVC.)

I'll see if that fixes it. Where should we put this, assuming it does fix it? (Does it need to go in TreeBuilder.java somehow?)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix (C++ only)

Yup, that #pragma fixes it (with quotes added around the warning message). Here's a patch.

Henri, let me know what else needs to be done, in terms of updating the Java code/translator/etc.

Comment 4

[Daniel Holbert [:dholbert]]

(Sorry, that patch has some extra chunks -- adding back the null-checks -- since I layered it on top of my other just-get-the-thing-to-build patch which removed then null-checks.  Just ignore those parts.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (C++ only)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v3 (C++ only)

Comment 7

[Henri Sivonen (:hsivonen)]

Even generating the pragma requires a change to the generator, so I figured I'd just make the generator produce idiomatic C++ for this. Sorry about the forward-duping, but Ehsan has a patch in bug 1109311 that can land without revision, so duplicating this against that one.

Comment 8

[Daniel Holbert [:dholbert]]

Thanks! ehsan++

Comment 9

[Henri Sivonen (:hsivonen)]

Comment on attachment 8498547 [details] [diff] [review]
fix v3 (C++ only)

Setting to feedback-, since this was fixed in another manner in the other bug.