Last Comment Bug 711480 - malformed conditional in js/src/shell/js.cpp
: malformed conditional in js/src/shell/js.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 8 Branch
: x86 Linux
: -- normal (vote)
: mozilla12
Assigned To: David Mandelin [:dmandelin]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-16 08:17 PST by Julia Lawall
Modified: 2011-12-27 09:49 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Julia Lawall 2011-12-16 08:17:46 PST
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] (remove +bmo to email) 2011-12-17 14:42:55 PST
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] 2011-12-17 14:46:42 PST
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] (remove +bmo to email) 2011-12-17 14:48:51 PST
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] 2011-12-19 00:23:30 PST
GCC has -Wempty-body, which is enabled by -Wextra but not -Wall.
Comment 5 Brendan Eich [:brendan] 2011-12-20 12:13:23 PST
(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 [:njn] (on vacation until July 11) 2011-12-20 12:58:13 PST
I'm going to add -Wempty-body in bug 711895.
Comment 7 Julia Lawall 2011-12-20 13:05:08 PST
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 [:njn] (on vacation until July 11) 2011-12-20 13:48:14 PST
(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] 2011-12-21 17:05:06 PST
Landed with visual review from dvander:

http://hg.mozilla.org/integration/mozilla-inbound/rev/5c40bfc44022
Comment 10 Ed Morley [:emorley] 2011-12-22 03:48:06 PST
https://hg.mozilla.org/mozilla-central/rev/5c40bfc44022
Comment 11 Emanuel Hoogeveen [:ehoogeveen] 2011-12-27 09:49:55 PST
(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

Note You need to log in before you can comment on or make changes to this bug.