Last Comment Bug 696813 - preparation patches for parser, emitter, decompiler
: preparation patches for parser, emitter, decompiler
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 701227 701239 701244
Blocks: 692274
  Show dependency treegraph
 
Reported: 2011-10-24 11:16 PDT by Luke Wagner [:luke]
Modified: 2012-01-12 18:42 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
forDecl (8.17 KB, patch)
2011-10-24 11:17 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
Emit* (3.81 KB, patch)
2011-10-24 11:17 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
forDecl (v2) (8.17 KB, patch)
2011-10-28 16:46 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review
Emit* (v2) (3.95 KB, patch)
2011-10-28 16:47 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review
rm the 'nextop' parameter from Decompile (12.30 KB, patch)
2011-10-28 16:48 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review
tidy up Decompile (12.84 KB, patch)
2011-10-28 16:50 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review
hoist two functions out of Decompile for reuse in later patch (4.30 KB, patch)
2011-10-28 16:51 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review
hoist another function for reuse in later patch (4.49 KB, patch)
2011-10-28 16:52 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-10-24 11:16:28 PDT
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] 2011-10-24 11:17:00 PDT
Created attachment 569113 [details] [diff] [review]
forDecl
Comment 2 Luke Wagner [:luke] 2011-10-24 11:17:29 PDT
Created attachment 569114 [details] [diff] [review]
Emit*
Comment 3 Luke Wagner [:luke] 2011-10-24 16:01:23 PDT
Green on try
Comment 4 Luke Wagner [:luke] 2011-10-28 16:46:14 PDT
Created attachment 570414 [details] [diff] [review]
forDecl (v2)

Refreshed
Comment 5 Luke Wagner [:luke] 2011-10-28 16:47:17 PDT
Created attachment 570415 [details] [diff] [review]
Emit* (v2)

Refreshed
Comment 6 Luke Wagner [:luke] 2011-10-28 16:48:15 PDT
Created attachment 570417 [details] [diff] [review]
rm the 'nextop' parameter from Decompile

... it's unused.
Comment 7 Luke Wagner [:luke] 2011-10-28 16:50:54 PDT
Created attachment 570418 [details] [diff] [review]
tidy up Decompile

Manual new/free -> Vector, removing goto, dead cases.
Comment 8 Luke Wagner [:luke] 2011-10-28 16:51:50 PDT
Created attachment 570419 [details] [diff] [review]
hoist two functions out of Decompile for reuse in later patch
Comment 9 Luke Wagner [:luke] 2011-10-28 16:52:31 PDT
Created attachment 570421 [details] [diff] [review]
hoist another function for reuse in later patch
Comment 10 Jason Orendorff [:jorendorff] 2011-11-02 15:44:21 PDT
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] 2011-11-02 15:48:17 PDT
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] 2011-11-02 15:57:22 PDT
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] 2011-11-02 16:19:21 PDT
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] 2011-11-03 08:16:37 PDT
(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] (remove +bmo to email) 2011-11-03 16:31:59 PDT
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] 2011-11-07 10:17:30 PST
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] 2011-11-07 11:49:36 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.