711480 - malformed conditional in js/src/shell/js.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Julia Lawall]

User Agent: Mozilla/5.0 (Ubuntu; X11; Linux i686; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111115183158

Steps to reproduce:

The following malformed conditional was found using Coccinelle (http://coccinelle.lip6.fr) in the file js/src/shell/js.cpp in the function ConvertArgs.  The ; after the if test means that ReportException is always called.

    if (!tmpstr || !func.encode(cx, tmpstr));
        ReportException(cx);

Comment 1

[Jeff Walden [:Waldo]]

Nice catch.

Some of the error-handling code in that method looks dodgy, but not enough to be sure that it's not that way intentionally so, so I'm not sure the obvious fix -- removing the ; -- is enough to be correct here.

Comment 2

[Brendan Eich [:brendan]]

Some compilers give a warning here -- this is a good one, worth error'ing on. SpiderMonkey house style uses continue; and not just ; to write empty loop bodies.

/be

Comment 3

[Jeff Walden [:Waldo]]

Brendan, that's not even a loop -- just an if statement.  I'm *really* not sure why compilers aren't warning about that.

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

GCC has -Wempty-body, which is enabled by -Wextra but not -Wall.

Comment 5

[Brendan Eich [:brendan]]

(In reply to Jeff Walden (remove +bmo to email) from comment #3)
> Brendan, that's not even a loop -- just an if statement.

Yes, I know -- it ought to be always an error.

> I'm *really* not sure why compilers aren't warning about that.

Walso, should comment 4 be spun out as a new owned bug? Or fixed here, whatever is best.

/be

Comment 6

[Nicholas Nethercote [inactive]]

I'm going to add -Wempty-body in bug 711895.

Comment 7

[Julia Lawall]

There are about 50 empty-bodied ifs in ipc/chromium/src/base/third_party/purify/pure_api.c.
These all look intentional.

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Julia Lawall from comment #7)
> There are about 50 empty-bodied ifs in
> ipc/chromium/src/base/third_party/purify/pure_api.c.
> These all look intentional.

Yes, but that file is only compiled on Windows so if you turn on -Wempty-body for GCC it doesn't matter.  I only get two extra warnings when I turn it on for Linux64 debug builds, both in gfx/ycbcr/yuv_convert.cpp.

Comment 9

[David Mandelin [:dmandelin]]

Landed with visual review from dvander:

http://hg.mozilla.org/integration/mozilla-inbound/rev/5c40bfc44022

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5c40bfc44022

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> (In reply to Julia Lawall from comment #7)
> > There are about 50 empty-bodied ifs in
> > ipc/chromium/src/base/third_party/purify/pure_api.c.
> > These all look intentional.
> 
> Yes, but that file is only compiled on Windows so if you turn on
> -Wempty-body for GCC it doesn't matter.  I only get two extra warnings when
> I turn it on for Linux64 debug builds, both in gfx/ycbcr/yuv_convert.cpp.

In case this comes up in the future, you can temporarily disable warnings in GCC by wrapping affected code with

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wempty-body"

#pragma GCC diagnostic pop