Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 7 months ago by Dmitry Lomov (no reviews) Modified:
5 years, 7 months ago Reviewers:
caitp (gmail), arv (Not doing code reviews), rossberg CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[destructuring] More tests for object literal pattern
R=arv@chromium.org,rossberg@chromium.org
BUG=v8:811
LOG=N
Committed: https://crrev.com/cf9492ddd818f336a9dc1269cdb7557ec709d12c
Cr-Commit-Position: refs/heads/master@{#28457}
Patch Set 1 #Patch Set 2 : More tests + fixed a bug #
Total comments: 11
Patch Set 3 : CR feedback #Patch Set 4 : Typo in error message fixed. #Patch Set 5 : Removed undetectable check #
Total comments: 4
Patch Set 6 : More feedback #
Total comments: 16
Patch Set 7 : Comments addressed, landing #
Total comments: 1
Created: 5 years, 7 months ago
Messages
Total messages: 30 (9 generated)
ptal
On 2015/05/18 09:52:27, Dmitry Lomov (chromium) wrote: > ptal (sorry seems like I forgot to push a button on Friday)
The CQ bit was checked by dslomov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139773005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
caitpotter88@gmail.com changed reviewers: + caitpotter88@gmail.com
https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de... File test/mjsunit/harmony/destructuring.js (right): https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de... test/mjsunit/harmony/destructuring.js:140: assertThrows(function() { var {} = null; }, TypeError); do we want to test the other cases that would cause the `RequireObjectCoercible(value)` test to throw? Including nested patterns
https://codereview.chromium.org/1139773005/diff/20001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1139773005/diff/20001/src/parser.cc#newcode4137 src/parser.cc:4137: // if (var === null || var === undefined || %_IsUndetectableObject(var)) Is this right? Why can I not do? let {someId} = document.all; The ES spec knows nothing about undetectable objects. https://codereview.chromium.org/1139773005/diff/20001/src/pattern-rewriter.cc File src/pattern-rewriter.cc (right): https://codereview.chromium.org/1139773005/diff/20001/src/pattern-rewriter.cc... src/pattern-rewriter.cc:220: block_->AddStatement(descriptor_->parser->BuildAssertIsCoercible(temp), In theory this is only need when we have an empty pattern. let {x} = null; => tmp = null; tmp.x = x; // null is not coercible Maybe add a TODO? https://codereview.chromium.org/1139773005/diff/20001/src/token.h File src/token.h (right): https://codereview.chromium.org/1139773005/diff/20001/src/token.h#newcode64 src/token.h:64: T(ASSIGN_ADD, "+ASSIGN_ADD=", 2) \ Why? https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de... File test/mjsunit/harmony/destructuring.js (right): https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de... test/mjsunit/harmony/destructuring.js:26: var o = { get x() { var o = { get x() { ... } }; https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de... test/mjsunit/harmony/destructuring.js:37: var { x : x0, y : { z : z1 }, x : x1 } = o; who would have ever thought :-)
Comments addressed. Sorry, I have no idea what is the meaning of 'undetectable' :( https://codereview.chromium.org/1139773005/diff/20001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1139773005/diff/20001/src/parser.cc#newcode4137 src/parser.cc:4137: // if (var === null || var === undefined || %_IsUndetectableObject(var)) On 2015/05/18 13:12:53, arv wrote: > Is this right? Why can I not do? > > let {someId} = document.all; > > The ES spec knows nothing about undetectable objects. The truth is, I have just copied logic from CHECK_OBJECT_COERCIBLE in macros.py. Tell me what the right semantics is. https://codereview.chromium.org/1139773005/diff/20001/src/pattern-rewriter.cc File src/pattern-rewriter.cc (right): https://codereview.chromium.org/1139773005/diff/20001/src/pattern-rewriter.cc... src/pattern-rewriter.cc:220: block_->AddStatement(descriptor_->parser->BuildAssertIsCoercible(temp), On 2015/05/18 13:12:53, arv wrote: > In theory this is only need when we have an empty pattern. > > let {x} = null; > > => > > tmp = null; > tmp.x = x; // null is not coercible > > Maybe add a TODO? Done. https://codereview.chromium.org/1139773005/diff/20001/src/token.h File src/token.h (right): https://codereview.chromium.org/1139773005/diff/20001/src/token.h#newcode64 src/token.h:64: T(ASSIGN_ADD, "+ASSIGN_ADD=", 2) \ On 2015/05/18 13:12:53, arv wrote: > Why? Oops. Undone (it's pathetic that no tests fail :() https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de... File test/mjsunit/harmony/destructuring.js (right): https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de... test/mjsunit/harmony/destructuring.js:26: var o = { get x() { On 2015/05/18 13:12:53, arv wrote: > var o = { > get x() { > ... > } > }; Done. https://codereview.chromium.org/1139773005/diff/20001/test/mjsunit/harmony/de... test/mjsunit/harmony/destructuring.js:140: assertThrows(function() { var {} = null; }, TypeError); On 2015/05/18 13:11:06, caitp wrote: > do we want to test the other cases that would cause the > `RequireObjectCoercible(value)` test to throw? Including nested patterns Done.
Removed undetectable check. Whatever we do, we must be consistent betweet
let {x} = foo
and
let x = foo.x
The latter has no undetectable check, so the former shouldn't as well.
https://codereview.chromium.org/1139773005/diff/80001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1139773005/diff/80001/src/parser.cc#newcode4138 src/parser.cc:4138: // return %ThrowNonCoercible(var); Instead of creating a new runtime function you can use ThrowNewTypeError here. https://codereview.chromium.org/1139773005/diff/80001/test/mjsunit/harmony/de... File test/mjsunit/harmony/destructuring.js (right): https://codereview.chromium.org/1139773005/diff/80001/test/mjsunit/harmony/de... test/mjsunit/harmony/destructuring.js:72: let { x : x0, y : { z : z1 }, x : x1 } = o; This makes me wish we had a test that makes sure we do all the getters in the right order.
Thanks for the review, all great suggestions! PTAL https://codereview.chromium.org/1139773005/diff/80001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1139773005/diff/80001/src/parser.cc#newcode4138 src/parser.cc:4138: // return %ThrowNonCoercible(var); On 2015/05/18 14:00:27, arv wrote: > Instead of creating a new runtime function you can use ThrowNewTypeError here. Done. https://codereview.chromium.org/1139773005/diff/80001/test/mjsunit/harmony/de... File test/mjsunit/harmony/destructuring.js (right): https://codereview.chromium.org/1139773005/diff/80001/test/mjsunit/harmony/de... test/mjsunit/harmony/destructuring.js:72: let { x : x0, y : { z : z1 }, x : x1 } = o; On 2015/05/18 14:00:27, arv wrote: > This makes me wish we had a test that makes sure we do all the getters in the > right order. Done.
(sorry had to rebase because NewThrowTypeError changed under me)
The CQ bit was checked by dslomov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139773005/100001
LGTM Some small improvements https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc#newcode4148 src/parser.cc:4148: // return throw /* type error kNonCoercible) */; Just throw, no return. https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc#newcode4165 src/parser.cc:4165: factory()->NewReturnStatement(throw_type_error, RelocInfo::kNoPosition), NewExpressionStatement https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.cc File src/pattern-rewriter.cc (right): https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.c... src/pattern-rewriter.cc:222: if (pattern->properties()->length() == 0) { That was easy https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d... File test/mjsunit/harmony/destructuring.js (right): https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d... test/mjsunit/harmony/destructuring.js:40: assertArrayEquals(["x", "y", "z", "x"], log); thanks https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d... test/mjsunit/harmony/destructuring.js:87: assertThrows(function() { x++; }, TypeError); Can you add a test/message/ test too? https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d... test/mjsunit/harmony/destructuring.js:98: +1 newline for consistency https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d... test/mjsunit/harmony/destructuring.js:132: (function TestExceptions() { How about: (function TestExceptions() { for (var val of [null, undefined]) { assertThrows(function() { var {} = val; }, TypeError); assertThrows(function() { var {x} = val; }, TypeError); assertThrows(function() { var { x : {} } = { x : val }; }, TypeError); assertThrows(function() { 'use strict'; let {} = val; }, TypeError); assertThrows(function() { 'use strict'; let {x} = val; }, TypeError); assertThrows(function() { 'use strict'; let { x : {} } = { x : val }; }, TypeError); } }());
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM modulo Arv's comments https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.cc File src/pattern-rewriter.cc (right): https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.c... src/pattern-rewriter.cc:223: block_->AddStatement(descriptor_->parser->BuildAssertIsCoercible(temp), Nit: I'd just inline this method
Landing https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc#newcode4148 src/parser.cc:4148: // return throw /* type error kNonCoercible) */; On 2015/05/18 14:43:42, arv wrote: > Just throw, no return. Indeed, so much simpler! Awesomesauce. Done. https://codereview.chromium.org/1139773005/diff/100001/src/parser.cc#newcode4165 src/parser.cc:4165: factory()->NewReturnStatement(throw_type_error, RelocInfo::kNoPosition), On 2015/05/18 14:43:42, arv wrote: > NewExpressionStatement Done. https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.cc File src/pattern-rewriter.cc (right): https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.c... src/pattern-rewriter.cc:222: if (pattern->properties()->length() == 0) { On 2015/05/18 14:43:42, arv wrote: > That was easy :-) Esp. since undetectable garbage is gone. https://codereview.chromium.org/1139773005/diff/100001/src/pattern-rewriter.c... src/pattern-rewriter.cc:223: block_->AddStatement(descriptor_->parser->BuildAssertIsCoercible(temp), On 2015/05/18 16:23:18, rossberg wrote: > Nit: I'd just inline this method I think I will reuse it for ArrayPattern later on. https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d... File test/mjsunit/harmony/destructuring.js (right): https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d... test/mjsunit/harmony/destructuring.js:40: assertArrayEquals(["x", "y", "z", "x"], log); On 2015/05/18 14:43:42, arv wrote: > thanks Acknowledged. https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d... test/mjsunit/harmony/destructuring.js:87: assertThrows(function() { x++; }, TypeError); On 2015/05/18 14:43:42, arv wrote: > Can you add a test/message/ test too? Done. https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d... test/mjsunit/harmony/destructuring.js:98: On 2015/05/18 14:43:42, arv wrote: > +1 newline for consistency Done. https://codereview.chromium.org/1139773005/diff/100001/test/mjsunit/harmony/d... test/mjsunit/harmony/destructuring.js:132: (function TestExceptions() { On 2015/05/18 14:43:42, arv wrote: > How about: > > (function TestExceptions() { > for (var val of [null, undefined]) { > assertThrows(function() { var {} = val; }, TypeError); > assertThrows(function() { var {x} = val; }, TypeError); > assertThrows(function() { var { x : {} } = { x : val }; }, TypeError); > assertThrows(function() { 'use strict'; let {} = val; }, TypeError); > assertThrows(function() { 'use strict'; let {x} = val; }, TypeError); > assertThrows(function() { 'use strict'; let { x : {} } = { x : val }; }, > TypeError); > } > }()); Done.
The CQ bit was checked by dslomov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org, rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/1139773005/#ps120001 (title: "Comments addressed, landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139773005/120001
https://codereview.chromium.org/1139773005/diff/120001/test/message/destructu... File test/message/destructuring-modify-const.js (right): https://codereview.chromium.org/1139773005/diff/120001/test/message/destructu... test/message/destructuring-modify-const.js:9: x++; I was thinking a test where we have a failure in the pattern const {x, y, x} = {x: 1, y: 2}; for example
The CQ bit was unchecked by dslomov@chromium.org
The CQ bit was checked by dslomov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139773005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/cf9492ddd818f336a9dc1269cdb7557ec709d12c Cr-Commit-Position: refs/heads/master@{#28457} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
