Closed
Bug 344601
Opened 18 years ago
Closed 18 years ago
Function.prototype.toString turns let statements/expressions into let declarations
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.8.1
People
(Reporter: nanto, Assigned: mrbkap)
References
Details
(Keywords: verified1.8.1)
Attachments
(1 file, 3 obsolete files)
9.19 KB,
patch
|
brendan
:
review+
beltzner
:
approval1.8.1+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060713 Minefield/3.0a1 ToString/toSource of a function containing let statements/expressions gives let declarations instead of the let statemetns/expressions. Reproducible: Always Steps to Reproduce: js> function f() { var i = 1; let (i = 2) {} return i; } js> f(); 1 Actual Results: js> f.toString(); function f() { var i = 1; let i = 2; return i; } js> eval(f.toString()); js> f(); 2 Expected Results: js> f.toString(); function f() { var i = 1; let (i = 2) { } return i; } js> eval(f.toString()); js> f(); 1
Assignee | ||
Comment 1•18 years ago
|
||
Yeah, the decompiler needs some love.
Assignee: general → mrbkap
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla1.8.1
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•18 years ago
|
||
This patch works, but is not ready for review. I'll explain the problems later.
Assignee | ||
Comment 3•18 years ago
|
||
This even avoids the additional bytecode (thanks to brendan for pointing out how to avoid it). Sadly, it leaves |for (let i = i in foo);| in a sad shape, but that's a less severe bug.
Attachment #233346 -
Attachment is obsolete: true
Attachment #233685 -
Flags: review?(brendan)
Assignee | ||
Comment 4•18 years ago
|
||
Attachment #233685 -
Attachment is obsolete: true
Attachment #233694 -
Flags: review?(brendan)
Attachment #233685 -
Flags: review?(brendan)
Comment 5•18 years ago
|
||
Comment on attachment 233694 [details] [diff] [review] Better >+ /* >+ * We pop the scope around initializers for for-in loop variables and let >+ * statements/expressions. Let statements/expressions have a different >+ * sourcenote sequence, so test here to see which case we are really in. >+ */ >+ inLetHead = (popScope && !(tc->flags & TCF_IN_FOR_INIT)); Nit: don't need parens around rhs of = operator. >+ /* If this is a let head, emit and return a srcnote on the pop. */ >+ if (!inLetHead) { >+ *headNoteIndex = -1; >+ } else { >+ *headNoteIndex = js_NewSrcNote(cx, cg, SRC_DECL); >+ if (*headNoteIndex < 0) >+ return JS_FALSE; >+ } >+ > return !(pn->pn_extra & PNX_POPVAR) || js_Emit1(cx, cg, JSOP_POP) >= 0; Correct me if I'm wrong, but if (inLetHead), we shouldn't even consider emitting the pop here. If true, this is better written: >+ /* >+ * We pop the scope around initializers for for-in loop variables and let >+ * statements/expressions. Let statements/expressions have a different >+ * sourcenote sequence, so test here to see which case we are really in. >+ */ >+ inLetHead = (popScope && !(tc->flags & TCF_IN_FOR_INIT)); Nit: don't need parens around rhs of = operator. /* If this is a let head, emit and return a srcnote on the pop. */ if (inLetHead) { *headNoteIndex = js_NewSrcNote(cx, cg, SRC_DECL); return *headNoteIndex >= 0; } *headNoteIndex = -1; return !(pn->pn_extra & PNX_POPVAR) || js_Emit1(cx, cg, JSOP_POP) >= 0; > /* Let and for loop heads evaluate initializers in the outer scope. */ >- inHead = (pn2 != NULL || (cg->treeContext.flags & TCF_IN_FOR_INIT)); >+ popScope = (pn2 != NULL || (cg->treeContext.flags & TCF_IN_FOR_INIT)); > > JS_ASSERT(pn->pn_arity == PN_LIST); >- if (!EmitVariables(cx, cg, pn, inHead)) >+ if (!EmitVariables(cx, cg, pn, popScope, ¬eIndex)) > return JS_FALSE; > >+ if (noteIndex >= 0) >+ tmp = CG_OFFSET(cg); I say set tmp unconditionally (did GCC finally get this case right, and not warn?). /be
Comment 6•18 years ago
|
||
(In reply to comment #5) > /* If this is a let head, emit and return a srcnote on the pop. */ > if (inLetHead) { > *headNoteIndex = js_NewSrcNote(cx, cg, SRC_DECL); > return *headNoteIndex >= 0; > } D'oh -- I was confused into thinking that in this case, the JSOP_POP was emitted by the caller. Blake's added an assertion that the PNX_POPVAR flag is set, though, since the SRC_DECL counts on that. /be
Assignee | ||
Comment 7•18 years ago
|
||
Attachment #233694 -
Attachment is obsolete: true
Attachment #233702 -
Flags: review?(brendan)
Attachment #233694 -
Flags: review?(brendan)
Comment 8•18 years ago
|
||
Comment on attachment 233702 [details] [diff] [review] Best r=me, thanks. /be
Attachment #233702 -
Flags: review?(brendan)
Attachment #233702 -
Flags: review+
Attachment #233702 -
Flags: approval1.8.1?
Assignee | ||
Comment 9•18 years ago
|
||
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 10•18 years ago
|
||
nanto you rock! Checking in regress-344601.js; /cvsroot/mozilla/js/tests/js1_7/block/regress-344601.js,v <-- regress-344601.js initial revision: 1.1
Flags: in-testsuite+
Comment 11•18 years ago
|
||
Comment on attachment 233702 [details] [diff] [review] Best a=beltzner on behalf of drivers for the mozilla_1_8_branch
Attachment #233702 -
Flags: approval1.8.1? → approval1.8.1+
Comment 12•18 years ago
|
||
verified fixed 1.9 20060815 windows/mac(ppc|tel)/linux
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 14•18 years ago
|
||
I filed bug 348904 on the for-in issue.
Comment 15•18 years ago
|
||
verified fixed 1.8 20060818 windows/mac*/linuc
Keywords: fixed1.8.1 → verified1.8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•