Closed Bug 1073707 Opened 5 years ago Closed 5 years ago

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]

Categories

(Core :: HTML: Parser, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1109311

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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?)
Blocks: buildwarning
(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.)
(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?)
Attached patch fix (C++ only) (obsolete) — Splinter Review
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.
Attachment #8497662 - Flags: feedback?(hsivonen)
Attachment #8497664 - Flags: feedback?(hsivonen)
Attachment #8497664 - Attachment is obsolete: true
Attachment #8497664 - Flags: feedback?(hsivonen)
Attachment #8498547 - Flags: feedback?(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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1109311
Thanks! ehsan++
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.
Attachment #8498547 - Flags: feedback?(hsivonen) → feedback-
You need to log in before you can comment on or make changes to this bug.