Comments addressed + rebased on top of latest patch for
https://codereview.chromium.org/1139773005/
https://codereview.chromium.org/1146683002/diff/1/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/1146683002/diff/1/src/parser.h#newcode1015
src/parser.h:1015: Scope* TemporaryDeclarationScope() const {
On 2015/05/18 16:17:19, rossberg wrote:
Nit: is there a strong reason to say "Temporary" here?
Yes, it is a declaration scope for temporary variables (as opposed to
declaration scope for the variables pattern matcher declares, which is
controlled by DeclarationDescriptor and ultimately by whether this is a
lexical declaration or not). However I inlined into CreateTempVar.
https://codereview.chromium.org/1146683002/diff/1/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):
https://codereview.chromium.org/1146683002/diff/1/src/pattern-rewriter.cc#newcode210
src/pattern-rewriter.cc:210: Variable*
Parser::PatternRewriter::CreateTemp(Expression* value) {
On 2015/05/18 14:36:20, arv wrote:
CreateTempVar maybe?
Done.
https://codereview.chromium.org/1146683002/diff/1/src/pattern-rewriter.cc#newcode245
src/pattern-rewriter.cc:245: DCHECK(node->op() == Token::ASSIGN);
On 2015/05/18 14:36:20, arv wrote:
Maybe add a comment explaining the desugaring
let {<pattern> = init} = <current_value>
becomes
temp = <current_value>
<pattern> = temp === undefined ? init : temp
Done.
https://codereview.chromium.org/1146683002/diff/1/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1146683002/diff/1/src/preparser.h#newcode2909
src/preparser.h:2909: classifier->RecordBindingPatternError(
On 2015/05/18 14:36:20, arv wrote:
Just curious... Is the reason why you are not doing a
RecordAssignmentPatternError here because assignment patterns are n ot
yet
implemented?
Yes.
https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc#newcode6366
test/cctest/test-parsing.cc:6366: "x",
On 2015/05/18 14:36:20, arv wrote:
this is a duplicate of the next line
Done.
https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc#newcode6392
test/cctest/test-parsing.cc:6392: "{var: x = 42}",
On 2015/05/18 16:17:19, rossberg wrote:
Add a test with computed property names.
I want to cover computed property names in a separate CL; this CL is
about intializers.
https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc#newcode6460
test/cctest/test-parsing.cc:6460: "{x : x += a}",
On 2015/05/18 16:17:19, rossberg wrote:
Add a test with method syntax, e.g. "{m() {} = 0}".
Done.
https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destructuring.js
File test/mjsunit/harmony/destructuring.js (right):
https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destructuring.js#newcode54
test/mjsunit/harmony/destructuring.js:54:
On 2015/05/18 14:36:20, arv wrote:
-1 newline
Done.
https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destructuring.js#newcode57
test/mjsunit/harmony/destructuring.js:57: for(var {z = 3} = {}; z != 0;
z--) {
On 2015/05/18 14:36:20, arv wrote:
ws
Done.
https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destructuring.js#newcode62
test/mjsunit/harmony/destructuring.js:62:
On 2015/05/18 16:17:20, rossberg wrote:
Nit: newline
Done.
https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destructuring.js#newcode66
test/mjsunit/harmony/destructuring.js:66: var o = { get x() {
On 2015/05/18 16:17:19, rossberg wrote:
Can we modify this test (and below) such that it also checks the order
of the
calls?
Done.
https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destructuring.js#newcode220
test/mjsunit/harmony/destructuring.js:220: (function
TestTDZInIntializers() {
On 2015/05/18 16:17:19, rossberg wrote:
Add tests like
let {x, y = eval("x")} = {x:42}
let {x = () => y, y} = {y:42}; x() === 42
let {x = () => eval("y"), y} = {y:42}; x() === 42
Done.
https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destructuring.js#newcode254
test/mjsunit/harmony/destructuring.js:254:
On 2015/05/18 14:36:20, arv wrote:
Did you want to add tests for
let {x, x} = {x: 1};
and some more variants of that:
let {x, x = 2} = {x: 1};
let {x = (function() { x = 2; })()} = {}; // error
let {x = (function() { x = 2; })()} = {x: 1}; // no error
Done.
https://codereview.chromium.org/1146683002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
