438986 - Some bad !js_Emit1(...) tests (sometimes even on left of < 0!)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

Attachment: fix

Patch says it all, also fixes some other warnings. Would like to land this ASAP.

/be

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 324897 [details] [diff] [review]
fix

Oops, Jason's off today, trying Igor.

/be

Comment 2

[Igor Bukanov]

(In reply to comment #0)
> Created an attachment (id=324897) [details]
> fix

Those --git patches are rather useless for review. Can you add the good old -pU8 version?

> Patch says it all, also fixes some other warnings.

It would be nice to have a proof that all the cases of bad usage are caught. I guess a long term solution would be to have a hydra script that checks that js_Emit1 is not used in a boolean context. Then the script can be run on one of tinderboxes together with all the other static checkers. But for this bug a simpler solution is to change js_Emit1 to return LikeInt defined as:

struct LikeInt
{
    int const value;
    LikeInt(int value_arg) : value(value_arg) { }
};

inline bool operator <(LikeInt a, int b)
{
    return a.value < b;
}
... // all the other compare operators 

Then the compiler must show all the places where theh results of js_Emit1 is used in a boolean context.

Comment 3

[Brendan Eich [:brendan]]

For now I can confirm no such bugs remain, via

$ grep '!js_Emit[^FT]' jsemit.cpp     
$

(F for FunctionScript and T for Tree).

Sorry about the diff format. I don't know how to get -p through from hg qdiff or hg diff -- Jason, any tips, or do we need to wait for your patch to make it into hg?

/be

Comment 4

[:Gavin Sharp [email: [e-mail]]]

As long as you're using hg 1.0, you can add:

[defaults]
diff = -U 8 -p

to your ~/.hgrc.

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to comment #3)

"hg diff -p" works in Mercurial 1.0.  "hg qdiff -p" doesn't.  But you can simulate qdiff using diff:

  hg diff -r qparent -U 8 -p

That's not identical to qdiff.  "qparent" is the revision on which all your patches are applied, so the result includes *all* applied MQ patches, not just the topmost one.

"hg qdiff -p" is in mercurial-crew, so I expect the next feature release (hg 1.1) will have it.  You can build mercurial-crew, if you like danger:
  hg clone http://hg.intevation.org/mercurial/crew/ mercurial-crew
  cd mercurial-crew
  cat README

Comment 6

[Brendan Eich [:brendan]]

Fixed:

changeset:   15428:268e926b48e1
tag:         tip
user:        Brendan Eich <[e-mail]>
date:        Wed Jun 18 18:50:33 2008 -0700
summary:     Fix bogus js_Emit return value tests (438986, r=igor).

/be