Closed
Bug 722121
Opened 13 years ago
Closed 13 years ago
Remove CheckRedeclaration
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(3 files)
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
CheckRedeclaration has no obvious parallel in the spec, and it confuses much more than it helps. There are very few uses of it, all of which can be addressed without too much trouble.
Assignee | ||
Comment 1•13 years ago
|
||
No functional change, just more closely paralleling the spec, and getting rid of the confusing CheckRedeclaration call that doesn't correspond to spec steps. Note that the algorithm isn't in a published spec PDF that I can find. <https://mail.mozilla.org/pipermail/es5-discuss/2011-January/003882.html> has a copy of it, and the most recent ES6 draft spec (at least) has it as well, but ES5.1, say, doesn't appear to. :-\
Attachment #592465 -
Flags: review?(jorendorff)
Assignee | ||
Comment 2•13 years ago
|
||
According to SemanticAnalysis.cpp and BytecodeEmitter.cpp, we can only generate JSOP_DEFFUN_FC if we have an optimizable function (not heavyweight, not in eval, etc.). Then, we can only have it if the function has upvars, and those upvars are flattenable. Given that JSOP_DEFFUN is only for global functions, the upvars would have to be global variables. We couldn't guarantee flattenability for vars, because they could be overwritten by a previous or subsequent script. We almost could for consts, but we don't. And there would be trouble there if the function in question were called before the const's initializer were reached. So I don't think it's possible to generate this opcode, and it should be removed. No JS tests or jit tests hit any of these new assertions, and some would fail if we were in a situation where JSOP_DEFFUN_FC would have been generated. Just to be clear, assuming this gets an r+, I have no plans to land it before the merge train departs for aurora.
Attachment #592468 -
Flags: review?(jorendorff)
Assignee | ||
Comment 3•13 years ago
|
||
There can be no conflict defining a getter or setter in an object literal, because literal syntax forbids conflicts: http://whereswalden.com/2011/01/09/new-es5-requirement-getters-and-setters-in-object-literals-must-not-conflict-with-each-other-or-with-data-properties-even-outside-strict-mode/ There can be no worry about the definition failing due to a readonly or permanent property because such a property can't exist on an object literal at execution time. Until the object is exposed to script, it's filled only with writable, configurable data properties. And now that sharps are gone, the object isn't exposed until after the entire literal is evaluated, and getters and setters would have been defined.
Attachment #592469 -
Flags: review?(jorendorff)
Comment 4•13 years ago
|
||
Comment on attachment 592465 [details] [diff] [review] Remove JSOP_DEF{VAR,CONST} uses This looks like the same code twice, except for THROW(). If that's the case, would you mind commoning up the code in an inline method called from two places? stubs::DefVarOrConst could just read if (!js::DefVarOrConstOperation(f.cx, *f.pc(), dn, f.fp())) THROW(); You are right, CheckRedeclaration does make this harder to follow than it should be.
Attachment #592465 -
Flags: review?(jorendorff) → review+
Comment 5•13 years ago
|
||
Comment on attachment 592468 [details] [diff] [review] Remove JSOP_DEFFUN_FC >+ MOZ_ASSERT(fn->getOp() != JSOP_DEFFUN, >+ "function statements at top level can never be fl[...] >+ "variables they might have can't be flattened"); Code like this: { let x = 42; function f() { return x; } } would produce a flat closure, but it happens that the TreeContext here is marked as TCF_FUN_HEAVYWEIGHT--which I think is just a bug. Global contexts should never have any of the TCF_FUN_* flags. The code that does this is js::Parser::functionDef: /* * If this function is not at body level of a program or function (i.e. * it is a function statement that is not a direct child of a program * or function), then our enclosing function, if any, must be * heavyweight. */ if (!bodyLevel && kind == Statement) outertc->flags |= TCF_FUN_HEAVYWEIGHT; Removing JSOP_DEFFUN_FC is fine with me, but the logic is kind of brittle. Let's not have this code in SemanticAnalysis.cpp depend on how the parser sets some flag. Instead make SemanticAnalysis.cpp explicitly test for JSOP_DEFFUN and say "no, we can't flatten that".
Attachment #592468 -
Flags: review?(jorendorff) → review+
Updated•13 years ago
|
Attachment #592469 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 6•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb8e2ee24071 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa99b3a04938 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1c5d3a29ff
Target Milestone: --- → mozilla13
Comment 7•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bb8e2ee24071 https://hg.mozilla.org/mozilla-central/rev/fa99b3a04938 https://hg.mozilla.org/mozilla-central/rev/6d1c5d3a29ff
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
•