311583 - uneval(array) should use elision, not undefined for holes in array

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

[ This is the second problem from the bug 311568 where the second part is a
duplicate of bug 260106 ]

Currently uneval in SpiderMonkey use undefined literal both for elements with
undefined value and non-existing elements or holes. For example, given:

var a = new Array(3); a[0] = a[2] = 0; 

SpiderMonkey gives for uneval(a) "[0, undefined, 0]" while expected value is
"[0, , 0]" which would properly capture the hole.

Comment 1

[Bob Clary [:bc] (inactive)]

Checking in regress-311583.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-311583.js,v  <--  regress-311583.js
initial revision: 1.1

Comment 2

[Igor Bukanov]

Attachment: Fix

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 203151 [details] [diff] [review]
Fix

Nits only, listed below with context.  Thanks for fixing this.

One non-nit-picking thought is to comment on how the static JSStrings are never tagged as jsvals or put where the GC might scan them.  It's old code that merely got moved, but I regret not writing a comment like that, so if you are game, I'd be grateful for it now.

/be

>     JSBool ok;
>     jsuint length, index;
>     jschar *chars, *ochars;
>     size_t nchars, growth, seplen, tmplen;
>     const jschar *sepstr;
>     JSString *str;
>     JSHashEntry *he;
>     JSObject *obj2;
>     int stackDummy;
>+    jsid id;

Convention puts stackDummy last, and other vars in order of first def in code, so maybe move up to under sepstr?

>+            if (id == JSID_HOLE) {
>+                str = (index + 1 != length ? cx->runtime->emptyString
>+                       : &comma_space);

Prevailing style evident in context later would put ? and the "then" operand on its own line, and make the ? and : line up under the (, not the i in index.

/be

Comment 4

[Igor Bukanov]

Attachment: New version of fix: no more JSString comma

Given the danger of storing accidentally "comma" and "comma_space" in GC-scannable area, the new patch eliminates them. As a replacement NULL is passed to array_join_sub which treats it a special value to indicate to use "," or ", " as separator depending on "literalize" flag.

To simplify code the patch changes the loop logic in array_join_sub to add the separator after each element except the last and not before each element except the first.

Comment 5

[Igor Bukanov]

With the last patch "var a = new Array(1); uneval(a)" gives "[, ]". IMO "[ ,]" would be more aesthetically pleasing. That is, the idea is to use just comma and not comma-blank after the tail hole.

What do you think?

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 203243 [details] [diff] [review]
New version of fix: no more JSString comma

Igor, I like [,] better than [, ] a little bit (but not [ ,] -- did you mean to put that space before the only comma?).  Only a little bit, so I'm fine with not polishing with extra code here.

Nits below.

>+/*
>+ * Here sep == NULL indicates to use a default separator which is "," or ", "
>+ * when literalize is true.
>+ */
> static JSBool
> array_join_sub(JSContext *cx, JSObject *obj, JSString *sep, JSBool literalize,
>                jsval *rval, JSBool localeString)

Rogerl added localeString after rval, but normal order puts out params last.  Now that we have the chance to change all callers, I'd rather not only reorder, but combine JSBools into a uintN flags param with JOIN_LITERAL and JOIN_LOCALE or some such flags.  The callers would be much more readable that way.  What do you think?

>   done:
>     if (literalize) {
>         if (chars) {
>-            if (JSVAL_IS_VOID(v)) {
>-                chars[nchars++] = ',';
>-                chars[nchars++] = ' ';
>-            }
>             chars[nchars++] = ']';
>         }

Nit: prevailing style does not over-brace single-line then clause of single-line if (condition), so this should collapse to just

        if (chars)
            chars[nchars++] = ']';

/be

Comment 7

[Igor Bukanov]

Attachment: Fix and fusing literalize/localeString into op.

This version of the fix replaces "literalize" and "localeString" boolean arguments by single "op" which spans over a enum with 3 elements, TO_STRING, TO_LOCALE_STRING and TO_SOURCE.

The patch make sure that for array literals after the tail hole only single "," is added, not the full ", " separator so, for example,  uneval(Array(1)) gives "[,]" and not "[, ]". For compatibility with JS1.2 test suite the old behavioir is preserved when version is set to 1.2. The cost of this eye candy is lines 492-493 in the new jsarray.c:

                if (index + 1 == length && !JS_VERSION_IS_1_2(cx))
                    seplen = 1;

In addition the patch fixes a bug I presume for JS1.2 scripts where the code treated null and undefined in the same way as holes but did not add the terminating "," after the tail null. Thus previously for the following script:
  version(120)
  print([null])
js shell would print "[]" while the the patch makes it to print "[, ]".

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 203358 [details] [diff] [review]
Fix and fusing literalize/localeString into op.

>@@ -414,106 +427,156 @@
>                 realloc((ochars = chars), nchars * sizeof(jschar) + growth);
>             if (!chars) {
>                 free(ochars);
>                 goto done;
>             }
>         }
>         chars[nchars++] = '[';
>+        JS_ASSERT(sep == NULL);
>+        /* Use ", " as separator */

Nit: either blank line before this comment and . at end of sentence in it, *or* (better, I think), lower-case u in "Use" and put this comment after the ; in the line it applies to, tabbed over (space-filled tab of course!) one 8-space stop:

>+        sepstr = NULL;
>+        seplen = 2;
>+
>     } else {
>         /*
>          * Free any sharp variable definition in chars.  Normally, we would
>          * MAKE_SHARP(he) so that only the first sharp variable annotation is
>          * a definition, and all the rest are references, but in the current
>-         * case of (!literalize), we don't need chars at all.
>+         * case of (op != TO_SOURCE), we don't need chars at all.
>          */
>         if (chars)
>             JS_free(cx, chars);
>         chars = NULL;
>         nchars = 0;
>+        /*
>+         * Always allocate extra char for terminating 0.
>+         */
>+        extratail = 1;

In light of above, this comment needs a blank line before it, and can collapse to a one-line comment; *or* again it could even be tabbed over after the ; of the statement to which it applies, and no blank lines added.

> 
>         /* Return the empty string on a cycle as well as on empty join. */
>         if (IS_BUSY(he) || length == 0) {
>             js_LeaveSharpObject(cx, NULL);
>             *rval = JS_GetEmptyStringValue(cx);
>             return ok;
>         }
> 
>         /* Flag he as BUSY so we can distinguish a cycle from a join-point. */
>         MAKE_BUSY(he);
>+
>+        if (sep) {
>+            sepstr = JSSTRING_CHARS(sep);
>+            seplen = JSSTRING_LENGTH(sep);
>+        } else {
>+            sepstr = NULL;
>+            /* Use "," as separator. */
>+            seplen = 1;

Same nit.

r=me with nits picked, thanks for fixing all of this!

/be

Comment 9

[Igor Bukanov]

Attachment: Patch to commit with comment style nits reflected

Comment 10

[Igor Bukanov]

I committed the last fix

Comment 11

[Bob Clary [:bc] (inactive)]

This regressed js1_5/Expressions/regress-96526-delelem.js. Note to self, changing the test to match new behavior will show false negatives on the branch.

Checking in regress-96526-delelem.js;
/cvsroot/mozilla/js/tests/js1_5/Expressions/regress-96526-delelem.js,v  <--  regress-96526-delelem.js
new revision: 1.4; previous revision: 1.3

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed cvs debug trunk build windows/linux

Comment 13

[Brendan Eich [:brendan]]

Attachment: followup fix to avoid warning and DEBUG-only UMR

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 205087 [details] [diff] [review]
followup fix to avoid warning and DEBUG-only UMR

Checked in.

/be

Comment 15

[Bob Clary [:bc] (inactive)]

fixed by Bug 336373 on the 1.8.1 branch. 
verified fixed 1.8.1 with windows/macppc/linux 20060707