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]

RESOLVED DUPLICATE of bug 1109311

Status

()

Core
HTML: Parser
RESOLVED DUPLICATE of bug 1109311
4 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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?)
(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?)
Created attachment 8497662 [details] [diff] [review]
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.
Attachment #8497662 - Flags: feedback?(hsivonen)
Comment hidden (obsolete)
Comment hidden (obsolete)
Created attachment 8498547 [details] [diff] [review]
fix v3 (C++ only)
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
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1109311
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.