Closed Bug 356083 Opened 18 years ago Closed 17 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
minor

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: crowderbt)

References

Details

(Keywords: testcase)

Attachments

(2 files, 8 obsolete files)

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

The decompilation does not compile because of the quotes.
js> uneval({'' setter: function(){}}) 
({set ''() {}})
Assignee: general → crowder
Status: NEW → ASSIGNED
What is the expected result here?
As with bug 356106 and bug 355992, such cases must use the old setter: (or getter:) syntax.

/be
Component: JavaScript Engine → Build Config
Component: Build Config → JavaScript Engine
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.
brendan:  I think the this => 'this' problem in the original bug is different than what's happening in those bugs
In neither case is set this{} or set ''{} allowed in an object initialiser; both cases need to use the old syntax when decompiling.

/be
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
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)
Blocks: 356133
This patch fixes the decompilation as well as the obj_toSource problem described in bug 356133
Attachment #241945 - Flags: review?(brendan)
Attached patch picking my own nits (obsolete) — Splinter Review
Attachment #241945 - Attachment is obsolete: true
Attachment #241948 - Flags: review?(brendan)
Attachment #241945 - Flags: review?(brendan)
Attached patch Damn, one more nit (obsolete) — Splinter Review
Sorry for the bugspam
Attachment #241948 - Attachment is obsolete: true
Attachment #241949 - Flags: review?(brendan)
Attachment #241948 - Flags: review?(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
(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.
(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
Attached patch addressing brendan's remarks (obsolete) — Splinter Review
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.
Attachment #241949 - Attachment is obsolete: true
Attachment #242028 - Flags: review?(brendan)
Attachment #241949 - Flags: review?(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
Attachment #242028 - Flags: review?(brendan) → review+
(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
Attachment #242028 - Flags: review+ → review-
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.
Attachment #242028 - Attachment is obsolete: true
Attachment #242039 - Flags: review?(brendan)
The string stuff now also more closely matches the preprocessor conditions under which it ran before (just earlier)
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
Thanks for catching the OBJ_LOOKUP_PROPERTY() issue.  Does "OBJ_GET_PROPERTY()" not introduce the same locking issue?
Attachment #242039 - Attachment is obsolete: true
Attachment #242043 - Flags: review?(brendan)
Attachment #242039 - Flags: review?(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 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
Attachment #242043 - Flags: review?(brendan) → review+
Attached patch removing obvious comment (obsolete) — Splinter Review
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
Attachment #242043 - Attachment is obsolete: true
Whiteboard: [checkin needed]
mozilla/js/src/jsobj.c 3.297
mozilla/js/src/jsopcode.c 3.194
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
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})
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
*** Bug 357444 has been marked as a duplicate of this bug. ***
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?
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)
Attachment #242078 - Attachment is obsolete: true
Attachment #262578 - Flags: review?(mrbkap)
Blocks: 358594
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.
Attachment #262578 - Attachment is obsolete: true
Attachment #262588 - Flags: review?(mrbkap)
Attachment #262578 - Flags: review?(mrbkap)
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)
Attachment #262588 - Flags: review?(mrbkap) → review?(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
Attachment #262588 - Flags: review?(igor)
Attachment #262588 - Flags: review?(brendan)
Attachment #262588 - Flags: review+
Attachment #262588 - Flags: review?(igor) → review+
Landed again, better this time:

jsobj.c: 3.334
jsopcode.c: 3.228
Status: REOPENED → RESOLVED
Closed: 18 years ago17 years ago
Resolution: --- → FIXED
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.
Attachment #263435 - Flags: review?(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
Attachment #263435 - Flags: review?(brendan) → review+
Depends on: 379482
/cvsroot/mozilla/js/tests/js1_5/decompilation/regress-356083.js,v  <--  regress-356083.js
initial revision: 1.1
Flags: in-testsuite+
jsopcode.c: 3.230 is the warning fix
verified fixed 1.9.0 linux/mac* shell 2007-05-05
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: