696813 - preparation patches for parser, emitter, decompiler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Small preparatory patches for bug 692274.

The first patch uses an explicit 'forDecl' local var in Parser::forStatement instead of relying on the token kind (which is invalidated by bug 692274).

The second patch removes unnecessary and confusing outparams in EmitTree-hoisted Emit* functions.

Comment 1

[Luke Wagner [:luke]]

Attachment: forDecl

Comment 2

[Luke Wagner [:luke]]

Attachment: Emit*

Comment 3

[Luke Wagner [:luke]]

Green on try

Comment 4

[Luke Wagner [:luke]]

Attachment: forDecl (v2)

Refreshed

Comment 5

[Luke Wagner [:luke]]

Attachment: Emit* (v2)

Refreshed

Comment 6

[Luke Wagner [:luke]]

Attachment: rm the 'nextop' parameter from Decompile

... it's unused.

Comment 7

[Luke Wagner [:luke]]

Attachment: tidy up Decompile

Manual new/free -> Vector, removing goto, dead cases.

Comment 8

[Luke Wagner [:luke]]

Attachment: hoist two functions out of Decompile for reuse in later patch

Comment 9

[Luke Wagner [:luke]]

Attachment: hoist another function for reuse in later patch

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 570414 [details] [diff] [review]
forDecl (v2)

Review of attachment 570414 [details] [diff] [review]:
-----------------------------------------------------------------

Great, r=me with comments.

::: js/src/frontend/Parser.cpp
@@ +3738,5 @@
> +     * Set to true for following four cases:
> +     *   for (let x in ...)
> +     *   for (let x = ...)
> +     *   for (var x in ...)
> +     *   for (var x = ...)

Maybe you could say,

    /*
     * True if we have 'for (var/let/const ...)', except in the oddball case
     * where 'let' begins a let-expression in 'for (let (...) ...)'.
     */
    bool forDecl = false;

Note x could be a destructuring decl, and the keyword could be 'const', blah blah.

@@ +3747,1 @@
>      ParseNode *pn1;

Micro-nit: I think 'expr' is a misleading name here, since pn1 is still used if it turns out to be a decl there.

@@ +3753,5 @@
> +                reportErrorNumber(pn, JSREPORT_ERROR, JSMSG_BAD_FOR_EACH_LOOP);
> +                return NULL;
> +            }
> +
> +            /* No initializer -- set first kid of left sub-node to null. */

I would be OK with killing this comment, now that you've put a comment on the declaration of pn1. "first kid of left sub-node" is not the kind of thing I find useful.

@@ +3779,5 @@
> +            } else if (tt == TOK_LET) {
> +                let = true;
> +                (void) tokenStream.getToken();
> +                if (tokenStream.peekToken() == TOK_LP) {
> +                    pn1 = letBlock(JS_FALSE);

Huh. All this is old code, but why do we want `let` to be true even if this is just a let-expression and thus forDecl is false? Not a bug, as it happens--just goofy. Consider moving this `let = true` down to the line where you do `forDecl = true`.

@@ +3879,5 @@
>                   * 'const' to hoist the initializer or the entire decl out of
>                   * the loop head. TOK_VAR is the type for both 'var' and 'const'.
>                   */
>  #if JS_HAS_BLOCK_SCOPE
> +                if (let) {

I think the last sentence of the comment can be removed now; the type doesn't particularly matter here anymore.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 570415 [details] [diff] [review]
Emit* (v2)

Review of attachment 570415 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with a bit of a shudder.

::: js/src/frontend/BytecodeGenerator.cpp
@@ +5335,5 @@
>  }
>  
>  #if JS_HAS_BLOCK_SCOPE
>  static bool
> +EmitLet(JSContext *cx, CodeGenerator *cg, ParseNode *pn)

Thank you.

@@ +5459,5 @@
>  }
>  #endif
>  
>  static bool
> +EmitLexicalScope(JSContext *cx, CodeGenerator *cg, ParseNode *pn)

Good grief. The "before" code was just bizarre!

Thank you.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 570419 [details] [diff] [review]
hoist two functions out of Decompile for reuse in later patch

Review of attachment 570419 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comment addressed.

::: js/src/jsopcode.cpp
@@ +1896,5 @@
> +    uintN i = numAtoms;
> +    for (Shape::Range r = blockObj->lastProperty()->all(); !r.empty(); r.popFront()) {
> +        const Shape &shape = r.front();
> +        if (shape.isEmptyShape())
> +            break;

Hmm. The isEmptyShape() call isn't in the original code. I think the last two lines quoted here should just be removed. Shape::Range does not expose empty shapes. Right?

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 570421 [details] [diff] [review]
hoist another function for reuse in later patch

Review of attachment 570421 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsopcode.cpp
@@ +136,5 @@
>  /************************************************************************/
>  
>  #define COUNTS_LEN 16
>  
> +typedef Vector<char, 8> CharBuffer;

We also have 'js::TokenStream::CharBuffer', which is Vector<jschar, 32>. That seems less than ideal for grepping and such.

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #10)
>      * True if we have 'for (var/let/const ...)', except in the oddball case
>      * where 'let' begins a let-expression in 'for (let (...) ...)'.

To clarify, 'for (const x in y)' is banned but 'for (const k = 0;;)' is allowed.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 570418 [details] [diff] [review]
tidy up Decompile

Review of attachment 570418 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsopcode.cpp
@@ -1563,5 @@
>          LOAD_OP_DATA(pc);
>          lval = PopStr(ss, JSOP_NOP);
> -        todo = SprintCString(&ss->sprinter, lval);
> -        if (op == JSOP_POPN)
> -            return pc;

o/~ And a special case gone, a special case gone, a special case bites the dust! o/~

(Ahem.)

@@ -1577,5 @@
> -        atom = NULL;
> -        lval = NULL;
> -        if (op == JSOP_SETARG) {
> -            atom = GetArgOrVarAtom(jp, GET_SLOTNO(pc));
> -            LOCAL_ASSERT(atom);

Egad, another!

@@ +1575,4 @@
>              atom = GetArgOrVarAtom(jp, i);
>              LOCAL_ASSERT(atom);
> +            if (!QuoteString(&ss->sprinter, atom, 0))
> +                return NULL;

r-, not enough layers of abstraction here.  How was js_AtomToPrintableString not perfectly pellucid?  NEEDS MOAR LIFOALLOCSCOPES.

@@ +2732,4 @@
>                  for (Shape::Range r = obj->lastProperty()->all(); !r.empty(); r.popFront()) {
>                      const Shape &shape = r.front();
> +                    JS_ASSERT(shape.hasShortID());
> +                    LOCAL_ASSERT(shape.shortid >= 0 && shape.shortid < argc);

One LOCAL_ASSERT for each of these -- makes bugs get bucketed better, and someone seeing the assert doesn't have to wonder which condition actually was false.

@@ +2828,5 @@
>                      js_printf(jp, ") {\n");
>                      jp->indent += 4;
>                      len = 0;
>                      break;
> +                  default:;

Truly I don't care, but is this actually the common style for an empty default at the end of a switch?

Comment 16

[Luke Wagner [:luke]]

Thanks Jason; great comments!

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> Huh. All this is old code, but why do we want `let` to be true even if this
> is just a let-expression and thus forDecl is false? Not a bug, as it
> happens--just goofy. Consider moving this `let = true` down to the line
> where you do `forDecl = true`.

`let` is definitely getting killed in the real bug 692274 so I'll leave it as is for now.

(In reply to Jason Orendorff [:jorendorff] from comment #13)
> We also have 'js::TokenStream::CharBuffer', which is Vector<jschar, 32>.
> That seems less than ideal for grepping and such.

Agreed; going with DupBuffer for lack of better name springing to mind.

Comment 17

[Luke Wagner [:luke]]

(In reply to Jeff Walden (remove +bmo to email) from comment #15)
> Truly I don't care, but is this actually the common style for an empty
> default at the end of a switch?

grep shows a bunch of uses, so I guess so.  I don't care much either.

Comment 18

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ff5f4b598f
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc437cae4d66
https://hg.mozilla.org/integration/mozilla-inbound/rev/95533390d3be
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6c390aa3bf2
https://hg.mozilla.org/integration/mozilla-inbound/rev/371fd6f5daa4
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e2a2222b06

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b5ff5f4b598f
https://hg.mozilla.org/mozilla-central/rev/cc437cae4d66
https://hg.mozilla.org/mozilla-central/rev/95533390d3be
https://hg.mozilla.org/mozilla-central/rev/a6c390aa3bf2
https://hg.mozilla.org/mozilla-central/rev/371fd6f5daa4
https://hg.mozilla.org/mozilla-central/rev/48e2a2222b06