302531 - QuoteString doesn't deal with not outputting any string

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

QuoteString currently assumes that it will output at least one character, when
this is not true, we get garbage for a result. Easy testcase:
function f(e) {
  return <e {e}="" />;
}

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: pick some nits, too

Comment 2

[Mike Shaver (:shaver emeritus)]

Comment on attachment 190859 [details] [diff] [review]
pick some nits, too

r=shaver

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 190859 [details] [diff] [review]
pick some nits, too

>Index: jsopcode.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsopcode.c,v
>retrieving revision 3.88
>diff -p -U10 -r3.88 jsopcode.c
>--- jsopcode.c	28 Jul 2005 00:38:40 -0000	3.88
>+++ jsopcode.c	28 Jul 2005 17:25:25 -0000
>@@ -424,20 +424,26 @@ QuoteString(Sprinter *sp, JSString *str,
>             ok = Sprint(sp, "\\%c", (char)u[1]) >= 0;
>         else
>             ok = Sprint(sp, (c >> 8) ? "\\u%04X" : "\\x%02X", c) >= 0;
>         if (!ok)
>             return NULL;
>     }
> 
>     /* Sprint the closing quote and return the quoted string. */
>     if (quote && Sprint(sp, "%c", (char)quote) < 0)
>         return NULL;

Ultra-nit: it's generally "house style" to put one blank line before a major
comment such as this one you just added:

>+    /* 
>+     * If we haven't Sprint'd anything yet, Sprint an empty string so that
>+     * the OFF2STR below gives a valid result.
>+     */
>+    if (off == sp->offset && Sprint(sp, "") < 0)
>+        return NULL;
>     return OFF2STR(sp, off);
> }
> 
> JSString *
> js_QuoteString(JSContext *cx, JSString *str, jschar quote)
> {
>     void *mark;
>     Sprinter sprinter;
>     char *bytes;
>     JSString *escstr;
>+++ jsemit.c	28 Jul 2005 17:25:27 -0000
>@@ -4705,33 +4705,34 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
>       case TOK_XMLLIST:
>         if (pn->pn_op == JSOP_XMLOBJECT) {
>             ok = EmitAtomOp(cx, pn, pn->pn_op, cg);
>             break;
>         }
> 
>         JS_ASSERT(pn->pn_type == TOK_XMLLIST || pn->pn_count != 0);
>         switch (pn->pn_head->pn_type) {
>             case TOK_XMLETAGO:
>                 JS_ASSERT(0);
>+                /* FALL THROUGH */
>             case TOK_XMLPTAGC:
>             case TOK_XMLSTAGO:
>                 break;
>-
>             default:
>                 if (js_Emit1(cx, cg, JSOP_STARTXML) < 0)
>                     return JS_FALSE;
>         }

The case and default labels should be half-indented, so the statements aren't
overindented one c-basic-offset.

Thanks, sorry for the delayed drive-by.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Checked in.

Comment 5

[Bob Clary [:bc] (inactive)]

Checking in regress-302531.js;
/cvsroot/mozilla/js/tests/e4x/Expressions/regress-302531.js,v  <--  regress-302531.js
initial revision: 1.1
done

Comment 6

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060818 win/mac*/linux