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

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Julia Lawall, Assigned: dmandelin)

Tracking

8 Branch
mozilla12
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

5 years ago
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);

Updated

5 years ago
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Brendan, that's not even a loop -- just an if statement.  I'm *really* not sure why compilers aren't warning about that.
GCC has -Wempty-body, which is enabled by -Wextra but not -Wall.
(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
I'm going to add -Wempty-body in bug 711895.
(Reporter)

Comment 7

5 years ago
There are about 50 empty-bodied ifs in ipc/chromium/src/base/third_party/purify/pure_api.c.
These all look intentional.
(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.
(Assignee)

Comment 9

5 years ago
Landed with visual review from dvander:

http://hg.mozilla.org/integration/mozilla-inbound/rev/5c40bfc44022
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/5c40bfc44022
Assignee: general → dmandelin
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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
You need to log in before you can comment on or make changes to this bug.