356083 - Incorrect decompilation for ({this setter: function () { } })

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> function() { return {this setter: function () { } }; }   
function () {
    return {set 'this'() {}};
}

The decompilation does not compile because of the quotes.

Comment 1

[Jesse Ruderman]

js> uneval({'' setter: function(){}}) 
({set ''() {}})

Comment 2

[Brian Crowder]

What is the expected result here?

Comment 3

[Brendan Eich [:brendan]]

As with bug 356106 and bug 355992, such cases must use the old setter: (or getter:) syntax.

/be

Comment 4

[Jesse Ruderman]

Crowder pointed out that uneval of an object does not share much code with decompilation of object literals in functions.  So I split comment 1 into 356133.

Comment 5

[Brian Crowder]

brendan:  I think the this => 'this' problem in the original bug is different than what's happening in those bugs

Comment 6

[Brendan Eich [:brendan]]

In neither case is set this{} or set ''{} allowed in an object initialiser; both cases need to use the old syntax when decompiling.

/be

Comment 7

[Brian Crowder]

I guess what I'm struggling with on this bug and on bug 356106 is what criteria to use to determine that the old syntax is necessary (for both decompiling and for obj_toString)

Comment 8

[Brian Crowder]

Attachment: use old-style setter/getter for now-invalid identifiers

This patch fixes the decompilation as well as the obj_toSource problem described in bug 356133

Comment 9

[Brian Crowder]

Attachment: picking my own nits

Comment 10

[Brian Crowder]

Attachment: Damn, one more nit

Sorry for the bugspam

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 241949 [details] [diff] [review]
Damn, one more nit

>+
>+        /*
>+         * Moved this chunk of code earlier so that we can make decisions
>+         * about old getter/setter versus new get/set syntax
>+         */

No need for historical ("Moved") comments in the code, CVS will remember.  But you might put a shorter version of this sentence after the next comment's content in a major-style comment:

>+        /* Convert id to a jsval and then to a string. */

        /*
         * Convert id to a jsval and then to a string.  Do this early so that
         * we can choose old getter/setter or new get/set syntax.
         */

>+        atom = JSID_IS_ATOM(id) ? JSID_TO_ATOM(id) : NULL;
>+        id = ID_TO_VALUE(id);
>+        idstr = js_ValueToString(cx, id);
>+        if (!idstr) {
>+            ok = JS_FALSE;
>+            goto error;
>+        }
>+        *rval = STRING_TO_JSVAL(idstr);         /* local root */
>+        needOldStyleGetterSetter = !js_IsIdentifier(idstr);

Might rename that flag idIsLexicalIdentifier or some such.

>-                    gsop[valcnt] =
>+                    gsop[valcnt] = needOldStyleGetterSetter ?
>+                        ATOM_TO_STRING(cx->runtime->atomState.getterAtom) :
>                         ATOM_TO_STRING(cx->runtime->atomState.getAtom);

Nit: multiline ?: style puts the ? and : at the start of the line, indented to underhang the first character of the condition (parenthesize the condition if it's not a member or primary expression -- here it's a primary so ok to leave unparenthesized).

> #endif
>                     valcnt++;
>                 }
>                 if (attrs & JSPROP_SETTER) {
>                     val[valcnt] = (jsval) ((JSScopeProperty *)prop)->setter;
> #ifdef OLD_GETTER_SETTER
>                     gsop[valcnt] =
>                         ATOM_TO_STRING(cx->runtime->atomState.setterAtom);
> #else
>-                    gsop[valcnt] =
>+                    gsop[valcnt] = needOldStyleGetterSetter ?
>+                        ATOM_TO_STRING(cx->runtime->atomState.setterAtom) :
>                         ATOM_TO_STRING(cx->runtime->atomState.setAtom);

Ditto.

>-        /* Convert id to a jsval and then to a string. */
>-        atom = JSID_IS_ATOM(id) ? JSID_TO_ATOM(id) : NULL;
>-        id = ID_TO_VALUE(id);
>-        idstr = js_ValueToString(cx, id);
>-        if (!idstr) {
>-            ok = JS_FALSE;
>-            goto error;
>-        }
>-        *rval = STRING_TO_JSVAL(idstr);         /* local root */
>-
>         /*
>          * If id is a string that's not an identifier, then it needs to be
>          * quoted.  Also, negative integer ids must be quoted.
>          */
>         if (atom
>             ? !js_IsIdentifier(idstr)

Common this by testing !idIsLexicalIdentifier.

>+            /* Not guarded by JS_HAS_GETTER_SETTER here?? */

No, minor code bloat, dead code since gsop[j] will be null.  Simpler that way, fewer ifdefs and no nested ifdefs.  No need to ask question in comment -- but you raise a good point: the comment you add here, or better yet an earlier one, should just say we don't #if JS_HAS_GETTER_SETTER gsop[j] tests in order to simplify the source code at the price of minor dead code bloat in the ECMA version (which is configured only for testing).

>-            if (gsop[j] && VALUE_IS_FUNCTION(cx, val[j])) {
>+            if (gsop[j] && VALUE_IS_FUNCTION(cx, val[j])
>+                && !needOldStyleGetterSetter) {

Prevailing style puts || and && at the end of the line, as context shows.  Also, not here but in other cases, if a condition spans multiple line, each && or || terms get its own line.  This can make for over-short lines as it would here, so judgment call.

>+            /* Not guarded by JS_HAS_GETTER_SETTER here?? */

Ditto.  This should just be removed in favor of a single earlier comment on #if avoidance.

>-            if (gsop[j]) {
>-                gsoplength = JSSTRING_LENGTH(gsop[j]);
>-                js_strncpy(&chars[nchars], JSSTRING_CHARS(gsop[j]), gsoplength);
>-                nchars += gsoplength;
>-                chars[nchars++] = ' ';
>+            if (needOldStyleGetterSetter) {
>+                js_strncpy(&chars[nchars], idstrchars, idstrlength);
>+                nchars += idstrlength;
>+                if (gsop[j]) {
>+                    chars[nchars++] = ' ';
>+                    gsoplength = JSSTRING_LENGTH(gsop[j]);
>+                    js_strncpy(&chars[nchars], JSSTRING_CHARS(gsop[j]),
>+                               gsoplength);
>+                    nchars += gsoplength;
>+                }
>+                chars[nchars++] = ':';
>+            } else {  /* New style "decompilation" */
>+                if (gsop[j]) {
>+                    gsoplength = JSSTRING_LENGTH(gsop[j]);
>+                    js_strncpy(&chars[nchars], JSSTRING_CHARS(gsop[j]),
>+                               gsoplength);
>+                    nchars += gsoplength;
>+                    chars[nchars++] = ' ';
>+                }
>+                js_strncpy(&chars[nchars], idstrchars, idstrlength);
>+                nchars += idstrlength;
>+                chars[nchars++] = gsop[j] ? ' ' : ':';

Won't this put produce 'get foo () {...}', i.e., with an unwanted space after the id?  Oh wait, I'm forgetting how the decompiler will take away that space, right?  That is must be there for the hardcoded math to work, IOW.  Worth a comment here.

>             }
>-            js_strncpy(&chars[nchars], idstrchars, idstrlength);
>-            nchars += idstrlength;
>-            chars[nchars++] = gsop[j] ? ' ' : ':';


>+++ jsopcode.c	11 Oct 2006 15:22:58 -0000
>@@ -3855,17 +3855,19 @@ Decompile(SprintStack *ss, jsbytecode *p
>                               (lastop == JSOP_GETTER || lastop == JSOP_SETTER)
>                               ? " " : "",
>                               (lastop == JSOP_GETTER) ? js_getter_str :
>                               (lastop == JSOP_SETTER) ? js_setter_str :
>                               "",
>                               rval);
> #else
>                 if (lastop == JSOP_GETTER || lastop == JSOP_SETTER) {
>-                    if (strncmp(rval, js_function_str, 8) || rval[8] != ' ') {
>+                    if (!ATOM_IS_IDENTIFIER(atom)
>+                        || strncmp(rval, js_function_str, 8)
>+                        || rval[8] != ' ') {

House style puts && and || at end; most other ops go on the continuation line.

/be

Comment 12

[Brian Crowder]

(In reply to comment #11)
> (From update of attachment 241949 [details] [diff] [review] [edit])
> 
> >+        needOldStyleGetterSetter = !js_IsIdentifier(idstr);
> 
> Might rename that flag idIsLexicalIdentifier or some such.
> 

I named it this because I anticipate potentially needing to add more criteria to it later, and I'd like this to be the only place I have to touch; additionally, I'd like it only to affect whether or not I use the old or new decompilation, and not things like this next IsIdentifier check.

> >         if (atom
> >             ? !js_IsIdentifier(idstr)
> 
> Common this by testing !idIsLexicalIdentifier.
> 



> >+            /* Not guarded by JS_HAS_GETTER_SETTER here?? */
> 
> No, minor code bloat, dead code since gsop[j] will be null.  Simpler that way,
> fewer ifdefs and no nested ifdefs.  No need to ask question in comment -- but
> you raise a good point: the comment you add here, or better yet an earlier one,
> should just say we don't #if JS_HAS_GETTER_SETTER gsop[j] tests in order to
> simplify the source code at the price of minor dead code bloat in the ECMA
> version (which is configured only for testing).

> >-            if (gsop[j]) {
> >-                gsoplength = JSSTRING_LENGTH(gsop[j]);
> >-                js_strncpy(&chars[nchars], JSSTRING_CHARS(gsop[j]), gsoplength);
> >-                nchars += gsoplength;
> >-                chars[nchars++] = ' ';
> >+            if (needOldStyleGetterSetter) {
> >+                js_strncpy(&chars[nchars], idstrchars, idstrlength);
> >+                nchars += idstrlength;
> >+                if (gsop[j]) {
> >+                    chars[nchars++] = ' ';
> >+                    gsoplength = JSSTRING_LENGTH(gsop[j]);
> >+                    js_strncpy(&chars[nchars], JSSTRING_CHARS(gsop[j]),
> >+                               gsoplength);
> >+                    nchars += gsoplength;
> >+                }
> >+                chars[nchars++] = ':';
> >+            } else {  /* New style "decompilation" */
> >+                if (gsop[j]) {
> >+                    gsoplength = JSSTRING_LENGTH(gsop[j]);
> >+                    js_strncpy(&chars[nchars], JSSTRING_CHARS(gsop[j]),
> >+                               gsoplength);
> >+                    nchars += gsoplength;
> >+                    chars[nchars++] = ' ';
> >+                }
> >+                js_strncpy(&chars[nchars], idstrchars, idstrlength);
> >+                nchars += idstrlength;
> >+                chars[nchars++] = gsop[j] ? ' ' : ':';
> 
> Won't this put produce 'get foo () {...}', i.e., with an unwanted space after
> the id?  Oh wait, I'm forgetting how the decompiler will take away that space,
> right?  That is must be there for the hardcoded math to work, IOW.  Worth a
> comment here.

What you see here for the new-style decompilation are spacing changes only.  I'll add this comment.

These fixes and other nits picked coming up shortly.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> I named it this because I anticipate potentially needing to add more criteria
> to it later, and I'd like this to be the only place I have to touch;
> additionally, I'd like it only to affect whether or not I use the old or new
> decompilation, and not things like this next IsIdentifier check.

Ok, but do capture idIsLexicalIdentifier somehow rather than recomputing it.

/be

Comment 14

[Brian Crowder]

Attachment: addressing brendan's remarks

Didn't quite fix the multiline ?: alignment nits:  lining up with the start of the condition pushes past 80 columns and doesn't suit the style of the similar expression above.

I also took the preprocessor guard away from the declarations for the two new variables, since the rest of the getter/setter stuff isn't fully guarded.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 242028 [details] [diff] [review]
addressing brendan's remarks

>+        JSBool needOldStyleGetterSetter = JS_FALSE;
>+        JSBool idIsLexicalIdentifier = JS_FALSE;
>+
>         /* Get strings for id and value and GC-root them via argv. */
>         id = ida->vector[i];
> 
> #if JS_HAS_GETTER_SETTER
> 
>         ok = OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop);
>         if (!ok)
>             goto error;
>+
>+        /*
>+         * Convert id to a jsval and then to a string.  Decide early whether we
>+         * prefer get/set or old getter/setter syntax.
>+         */
>+        atom = JSID_IS_ATOM(id) ? JSID_TO_ATOM(id) : NULL;
>+        id = ID_TO_VALUE(id);
>+        idstr = js_ValueToString(cx, id);
>+        if (!idstr) {
>+            ok = JS_FALSE;
>+            goto error;
>+        }
>+        *rval = STRING_TO_JSVAL(idstr);         /* local root */
>+        idIsLexicalIdentifier = js_IsIdentifier(idstr); /* cache result */
>+        needOldStyleGetterSetter = !idIsLexicalIdentifier;

No need to init either of these boolean vars to false, given the straight line code from their declarations to here, and the only discontinuities being goto error; statements.

>-                    gsop[valcnt] =
>-                        ATOM_TO_STRING(cx->runtime->atomState.getAtom);
>+                    gsop[valcnt] = needOldStyleGetterSetter
>+                        ? ATOM_TO_STRING(cx->runtime->atomState.getterAtom)
>+                        : ATOM_TO_STRING(cx->runtime->atomState.getAtom);

80th column trumps underhanging indentation ;-).

>+        /*
>+         * Hereafter, we don't #if JS_HAS_GETTER_SETTER, instead, we choose
>+         * to simplify the source code at the price of minor dead code bloat in
>+         * the ECMA version (which is configured only for testing), gsop[j]
>+         * being NULL will suffice.

Run-on sentence -- how about "we simplify the source code at the price of minor dead code bloat in the ECMA version (for testing only, see jsconfig.h).  The null default values in gsop[j] suffice to disable non-ECMA getter and setter code."

Or something like that (with two sentences).

r=me with those nits picked, stamp the patch yourself when attaching. Thanks,

/be

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> (From update of attachment 242028 [details] [diff] [review] [edit])
> >+        JSBool needOldStyleGetterSetter = JS_FALSE;
> >+        JSBool idIsLexicalIdentifier = JS_FALSE;
> >+
> >         /* Get strings for id and value and GC-root them via argv. */
> >         id = ida->vector[i];
> > 
> > #if JS_HAS_GETTER_SETTER
> > 
> >         ok = OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop);
> >         if (!ok)
> >             goto error;
> >+
> >+        /*
> >+         * Convert id to a jsval and then to a string.  Decide early whether we
> >+         * prefer get/set or old getter/setter syntax.
> >+         */
> >+        atom = JSID_IS_ATOM(id) ? JSID_TO_ATOM(id) : NULL;
> >+        id = ID_TO_VALUE(id);
> >+        idstr = js_ValueToString(cx, id);
> >+        if (!idstr) {
> >+            ok = JS_FALSE;
> >+            goto error;
> >+        }
> >+        *rval = STRING_TO_JSVAL(idstr);         /* local root */
> >+        idIsLexicalIdentifier = js_IsIdentifier(idstr); /* cache result */
> >+        needOldStyleGetterSetter = !idIsLexicalIdentifier;
> 
> No need to init either of these boolean vars to false, given the straight line
> code from their declarations to here, and the only discontinuities being goto
> error; statements.

Except as Brian pointed out, the #if JS_HAS_GETTER_SETTER could chop out the real initializations, so the JS_FALSE initializers are neeed, at least #if !JS_HAS_GETTER_SETTER.  Or you could #define those names to JS_FALSE in that case (ugly, but we do it elsewhere).  So the default init to false is ok.

But this points out that js_IsIdentifier won't be called at all if !JS_HAS_GETTER_SETTER.  That argues for early non-default init.

/be

Comment 17

[Brian Crowder]

Attachment: slight tweak to address concerns over redundant init and preproc complications

Brendan:  The only thing that's new is that I have taken the js_IsIdentifier check out of the HAS_GETTER_SETTER guard.  Nothing else is reordered and no other logic is moved.  This allows me not to initialize the needOldStyle or idIsIdent variables, so those inits are gone.

Comment 18

[Brian Crowder]

The string stuff now also more closely matches the preprocessor conditions under which it ran before (just earlier)

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 242039 [details] [diff] [review]
slight tweak to address concerns over redundant init and preproc complications

>+        JSBool needOldStyleGetterSetter, idIsLexicalIdentifier;

Order these by first-set (which happens to be alphabetical): idIs..., need....

>+
>         /* Get strings for id and value and GC-root them via argv. */
>         id = ida->vector[i];
> 
> #if JS_HAS_GETTER_SETTER
>-
>         ok = OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop);
>         if (!ok)
>             goto error;
>+#endif
>+
>+        /*
>+         * Convert id to a jsval and then to a string.  Decide early whether we
>+         * prefer get/set or old getter/setter syntax.
>+         */
>+        atom = JSID_IS_ATOM(id) ? JSID_TO_ATOM(id) : NULL;
>+        id = ID_TO_VALUE(id);
>+        idstr = js_ValueToString(cx, id);
>+        if (!idstr) {
>+            ok = JS_FALSE;
>+            goto error;

Oops, if prop != NULL you'll leave obj2 locked (fail to OBJ_DROP_PROPERTY(cx, obj2, prop)).  Do that here before the goto error.

>+        }
>+        *rval = STRING_TO_JSVAL(idstr);         /* local root */
>+        idIsLexicalIdentifier = js_IsIdentifier(idstr); /* cache result */
>+        needOldStyleGetterSetter = !idIsLexicalIdentifier;
>+
>+

Avoid two blank lines in a row.

>+#if JS_HAS_GETTER_SETTER
>+
>         valcnt = 0;
>         if (prop) {

Ok, now we deal with prop != NULL.

>             ok = OBJ_GET_ATTRIBUTES(cx, obj2, id, prop, &attrs);
>             if (!ok) {
>                 OBJ_DROP_PROPERTY(cx, obj2, prop);
>                 goto error;
>             }
>             if (OBJ_IS_NATIVE(obj2) &&
>                 (attrs & (JSPROP_GETTER | JSPROP_SETTER))) {
>                 if (attrs & JSPROP_GETTER) {
>                     val[valcnt] = (jsval) ((JSScopeProperty *)prop)->getter;
> #ifdef OLD_GETTER_SETTER
>                     gsop[valcnt] =
>                         ATOM_TO_STRING(cx->runtime->atomState.getterAtom);
> #else
>-                    gsop[valcnt] =
>-                        ATOM_TO_STRING(cx->runtime->atomState.getAtom);
>+                    gsop[valcnt] = needOldStyleGetterSetter
>+                        ? ATOM_TO_STRING(cx->runtime->atomState.getterAtom)
>+                        : ATOM_TO_STRING(cx->runtime->atomState.getAtom);
> #endif
>                     valcnt++;
>                 }
>                 if (attrs & JSPROP_SETTER) {
>                     val[valcnt] = (jsval) ((JSScopeProperty *)prop)->setter;
> #ifdef OLD_GETTER_SETTER
>                     gsop[valcnt] =
>                         ATOM_TO_STRING(cx->runtime->atomState.setterAtom);
> #else
>-                    gsop[valcnt] =
>-                        ATOM_TO_STRING(cx->runtime->atomState.setAtom);
>+                    gsop[valcnt] = needOldStyleGetterSetter
>+                        ? ATOM_TO_STRING(cx->runtime->atomState.setterAtom)
>+                        : ATOM_TO_STRING(cx->runtime->atomState.setAtom);
> #endif
>                     valcnt++;
>                 }
>             } else {
>                 valcnt = 1;
>                 gsop[0] = NULL;
>                 ok = OBJ_GET_PROPERTY(cx, obj, id, &val[0]);
>             }
>             OBJ_DROP_PROPERTY(cx, obj2, prop);
>         }
> 
> #else  /* !JS_HAS_GETTER_SETTER */
> 
>+        /*
>+         * Hereafter, we don't #if JS_HAS_GETTER_SETTER, instead, we choose

Still run-on: how about "we don't #if JS_HAS_GETTER_SETTER.  Instead, we simplify...."

/be

Comment 20

[Brian Crowder]

Attachment: fixing lock problem and a couple comments and nits

Thanks for catching the OBJ_LOOKUP_PROPERTY() issue.  Does "OBJ_GET_PROPERTY()" not introduce the same locking issue?

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #20)
> Created an attachment (id=242043) [edit]
> fixing lock problem and a couple comments and nits
> 
> Thanks for catching the OBJ_LOOKUP_PROPERTY() issue.  Does "OBJ_GET_PROPERTY()"
> not introduce the same locking issue?

No, because it does not have obj2 and prop out parameters.  OBJ_LOOKUP_PROPERTY has those out parameters so callers can (a) do existence checks and tell in which object the property exists; (b) call OBJ_GET_ATTRIBUTES or similar passing prop to avoid re-lookup.  An OBJ_DROP_PROPERTY must follow every OBJ_LOOKUP_PROPERTY that does not fail (return false) or not find the given id (prop == NULL after).

/be

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 242043 [details] [diff] [review]
fixing lock problem and a couple comments and nits

>+        *rval = STRING_TO_JSVAL(idstr);         /* local root */
>+        idIsLexicalIdentifier = js_IsIdentifier(idstr); /* cache result */

One last nit (I promise!): either line up the two minor comments to start in the same column (1 mod 4 starting from column 1), or lose the second (it's kind of obvious).

Also, just for kicks did you try making with JS_VERSION set to 148 in jsconfig.h after make clean?

r=me, thanks for nailing this -- good to have fewer #ifs.

/be

Comment 23

[Brian Crowder]

Attachment: removing obvious comment

No additional r+ needed here, this only removes a comment.

Brendan:  with JS_VERSION set to 148, the build is broken, but not in jsobj.c or jsopcode.c:

cc -o Darwin_DBG.OBJ/jsscan.o -c -Wall -Wno-format -g -DXP_UNIX -DSVR4 -DSYSV -D_BSD_SOURCE -DPOSIX_SOURCE -DDARWIN -DX86_LINUX  -DDEBUG -DDEBUG_crowder -DEDITLINE -IDarwin_DBG.OBJ  jsscan.c
jsscan.c: In function 'ReportCompileErrorNumber':
jsscan.c:567: error: dereferencing pointer to incomplete type
jsscan.c:581: error: dereferencing pointer to incomplete type
jsscan.c:596: error: dereferencing pointer to incomplete type

Comment 24

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

mozilla/js/src/jsobj.c 3.297
mozilla/js/src/jsopcode.c 3.194

Comment 25

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

I had to back this out, since it caused a startup crash on the tinderboxes:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1160709540.25915.gz

I also noticed the following broken behavior with this patch applied:
js> ({a:1}).toSource()
({a:undefined})

Comment 26

[Boris Zbarsky [:bzbarsky]]

*** Bug 357444 has been marked as a duplicate of this bug. ***

Comment 27

[Brian Crowder]

brendan:  I recall you telling me long ago that it is the ID_TO_VALUE code here that is breaking this patch.  Can you explain how/why, please?

Comment 28

[Brian Crowder]

Attachment: resurrected without dumb "id = ID_TO_VALUE(id)" code

This patch lays the groundwork for a fix I'd like to apply in bug 358594, so I felt it was worth reviving (with a fix for the bug pointed out by gavin -- the same bug which broke tinderboxes, hopefully)

Comment 29

[Brian Crowder]

Attachment: fixing the actual bug

The old me apparently didn't check whether this patch actually fixed the original bug (he did check the '' case, but not the "this" case).  The new me has a better patch.

Comment 30

[Brian Crowder]

Comment on attachment 262588 [details] [diff] [review]
fixing the actual bug

Brendan:  I think you may actually have more time for this than mrbkap (and you've seen it before)

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 262588 [details] [diff] [review]
fixing the actual bug


> /*
>  * Callers know that ATOM_IS_STRING(atom), and we leave it to the optimizer to
>  * common ATOM_TO_STRING(atom) here and near the call sites.
>  */
> #define ATOM_IS_IDENTIFIER(atom) js_IsIdentifier(ATOM_TO_STRING(atom))
>+#define ATOM_IS_KEYWORD(atom)                                                 \
>+            (js_CheckKeyword(JSSTRING_CHARS(ATOM_TO_STRING(atom)),            \
>+                             JSSTRING_LENGTH(ATOM_TO_STRING(atom))) != TOK_EOF)

Prevailing style would indent overflow lines of macro, if all body lines overflow (i.e., don't start on the #define line) by four spaces. This avoids hitting column 79 above, to boot.

The common subexpression ATOM_TO_STRING(atom), I trust, will be fused by all the compilers we care about.

I'm a little out of it today, so asking for second review from Igor.

/be

Comment 32

[Brian Crowder]

Landed again, better this time:

jsobj.c: 3.334
jsopcode.c: 3.228

Comment 33

[Brian Crowder]

Attachment: fixing a compiler warning

Brendan: I believe this fixes the compiler warning you e-mailed me about (I don't get it in opt anymore -- I never got it in debug, though), I am not sure if it is a correct fix.

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 263435 [details] [diff] [review]
fixing a compiler warning

This is a fix. The goto do_initprop; from JSOP_INITELEM can never follow (lastop) a JSOP_GETTER or JSOP_SETTER, so we know atom won't be used. nulling it helps us crash on th ATOM_IS_IDENTIFIER(atom) test (no need to assert if a clear crash is guaranteed).

Could move do_initprop: down to the else clause but the OLD_GETTER_SETTER #ifdef would have to go. Don't bother with that here.

/be

Comment 35

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/decompilation/regress-356083.js,v  <--  regress-356083.js
initial revision: 1.1

Comment 36

[Brian Crowder]

jsopcode.c: 3.230 is the warning fix

Comment 37

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac* shell 2007-05-05