Closed
Bug 696813
Opened 13 years ago
Closed 13 years ago
preparation patches for parser, emitter, decompiler
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(6 files, 2 obsolete files)
8.17 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
3.95 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
12.30 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
12.84 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
4.30 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
4.49 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #569113 -
Flags: review?(jorendorff)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #569114 -
Flags: review?(jorendorff)
Assignee | ||
Comment 3•13 years ago
|
||
Green on try
Assignee | ||
Comment 4•13 years ago
|
||
Refreshed
Attachment #569113 -
Attachment is obsolete: true
Attachment #569113 -
Flags: review?(jorendorff)
Attachment #570414 -
Flags: review?(jorendorff)
Assignee | ||
Comment 5•13 years ago
|
||
Refreshed
Attachment #569114 -
Attachment is obsolete: true
Attachment #569114 -
Flags: review?(jorendorff)
Attachment #570415 -
Flags: review?(jorendorff)
Assignee | ||
Updated•13 years ago
|
Attachment #570417 -
Attachment is patch: true
Assignee | ||
Comment 7•13 years ago
|
||
Manual new/free -> Vector, removing goto, dead cases.
Attachment #570418 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #570419 -
Flags: review?(jorendorff)
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #570421 -
Flags: review?(jorendorff)
Assignee | ||
Updated•13 years ago
|
Summary: simplify Parser::forStatement and some Emit* functions → preparation patches for parser, emitter, decompiler
Updated•13 years ago
|
Attachment #570417 -
Flags: review?(jwalden+bmo) → review+
Comment 10•13 years ago
|
||
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.
Attachment #570414 -
Flags: review?(jorendorff) → review+
Comment 11•13 years ago
|
||
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.
Attachment #570415 -
Flags: review?(jorendorff) → review+
Comment 12•13 years ago
|
||
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?
Attachment #570419 -
Flags: review?(jorendorff) → review+
Comment 13•13 years ago
|
||
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.
Attachment #570421 -
Flags: review?(jorendorff) → review+
Comment 14•13 years ago
|
||
(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•13 years ago
|
||
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?
Attachment #570418 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 16•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
(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.
Assignee | ||
Comment 18•13 years ago
|
||
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
Target Milestone: --- → mozilla10
Comment 19•13 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•