Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(8)

Issue 2606833002: [ESnext] Implement Object spread (Closed)

Created:
3 years, 12 months ago by gsathya
Modified:
3 years, 11 months ago
CC:
v8-reviews_googlegroups.com, rmcilroy
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[ESnext] Implement Object spread This patch adds parsing of spread object property. -- Changes ParsePropertyName to parse Token::ELLIPSIS. -- Throws if rest is encountered by setting a pattern error. -- Adds a new PropertyKind enum (SPREAD) -- Adds a new ObjectLiteralProperty::kind (SPREAD) -- Adds a new harmony-object-spread flag and protects the parser code with it. -- Adds a new runtime function called CopyDataProperties -- Does not add any support for this feature in fullcodegen. -- Ignition calls out to a runtime function CopyDataProperties to perform spread operation. -- Move FastAssign from builtins-objects.cc to objects.cc -- Refactor Builtin_ObjectAssign to use SetOrCopyDataProperties Object rest will be implemented in a follow on patch. BUG=v8:5549 Review-Url: https://codereview.chromium.org/2606833002 Cr-Commit-Position: refs/heads/master@{#42102} Committed: https://chromium.googlesource.com/v8/v8/+/a40b7172fe7d255b4ce3055d4ca7cdbbb3f09fb6

Patch Set 1 #

Patch Set 2 : flag + throw error on destructuring #

Patch Set 3 : fix backends #

Patch Set 4 : add tests #

Patch Set 5 : fixes #

Patch Set 6 : use is_dynamic_name #

Patch Set 7 : reorder switch #

Patch Set 8 : revert is_dynamic_name change #

Total comments: 26

Patch Set 9 : address review comments #

Patch Set 10 : fix build #

Patch Set 11 : fix build #

Total comments: 17

Patch Set 12 : add more tests #

Patch Set 13 : more tests #

Total comments: 16

Patch Set 14 : fixnits #

Patch Set 15 : fixes #

Total comments: 2

Patch Set 16 : fix test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+445 lines, -123 lines) Patch
M src/ast/ast.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -2 lines 0 comments Download
M src/ast/ast.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/ast/prettyprinter.cc View 1 1 chunk +3 lines, -0 lines 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -0 lines 0 comments Download
M src/builtins/builtins-object.cc View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -118 lines 0 comments Download
M src/compiler/ast-graph-builder.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -1 line 0 comments Download
M src/full-codegen/arm/full-codegen-arm.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/full-codegen/arm64/full-codegen-arm64.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/full-codegen/ia32/full-codegen-ia32.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/full-codegen/mips/full-codegen-mips.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/full-codegen/mips64/full-codegen-mips64.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/full-codegen/ppc/full-codegen-ppc.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/full-codegen/s390/full-codegen-s390.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/full-codegen/x64/full-codegen-x64.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/full-codegen/x87/full-codegen-x87.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/interpreter/bytecode-generator.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +8 lines, -0 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +6 lines, -0 lines 0 comments Download
M src/objects.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +149 lines, -0 lines 0 comments Download
M src/parsing/parser.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -0 lines 0 comments Download
M src/parsing/parser-base.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +35 lines, -1 line 0 comments Download
M src/runtime/runtime.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/runtime/runtime-object.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +16 lines, -0 lines 0 comments Download
M test/cctest/test-parsing.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +93 lines, -1 line 0 comments Download
A test/mjsunit/harmony/object-spread-basic.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +111 lines, -0 lines 0 comments Download

Messages

Total messages: 71 (57 generated)
gsathya
Working on mjsunit tests, but would like feedback on implementation + inspiration for negative parsing ...
3 years, 11 months ago (2016-12-28 23:47:27 UTC) #26
adamk
https://codereview.chromium.org/2606833002/diff/140001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2606833002/diff/140001/src/parsing/parser-base.h#newcode2118 src/parsing/parser-base.h:2118: BindingPatternUnexpectedToken(); I think you'll need to disallow this in ...
3 years, 11 months ago (2016-12-29 00:48:45 UTC) #32
gsathya
https://codereview.chromium.org/2606833002/diff/140001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2606833002/diff/140001/src/parsing/parser-base.h#newcode2118 src/parsing/parser-base.h:2118: BindingPatternUnexpectedToken(); On 2016/12/29 00:48:45, adamk wrote: > I think ...
3 years, 11 months ago (2016-12-29 06:40:57 UTC) #42
gsathya
+benedikt for backends
3 years, 11 months ago (2016-12-29 06:42:33 UTC) #44
Benedikt Meurer
Haven't looked at parsing, but the rest is LGTM. We should probably think about an ...
3 years, 11 months ago (2016-12-29 08:39:31 UTC) #47
Dan Ehrenberg
The code looks good here. A couple small points: Could you call out in the ...
3 years, 11 months ago (2016-12-29 17:31:04 UTC) #48
adamk
https://codereview.chromium.org/2606833002/diff/140001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2606833002/diff/140001/src/parsing/parser-base.h#newcode2118 src/parsing/parser-base.h:2118: BindingPatternUnexpectedToken(); On 2016/12/29 06:40:56, gsathya wrote: > On 2016/12/29 ...
3 years, 11 months ago (2016-12-29 18:32:46 UTC) #49
gsathya
https://codereview.chromium.org/2606833002/diff/140001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2606833002/diff/140001/src/parsing/parser-base.h#newcode2118 src/parsing/parser-base.h:2118: BindingPatternUnexpectedToken(); On 2016/12/29 18:32:45, adamk wrote: > On 2016/12/29 ...
3 years, 11 months ago (2017-01-05 22:18:36 UTC) #55
adamk
Looking good, just a few more comments. https://codereview.chromium.org/2606833002/diff/200001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2606833002/diff/200001/src/parsing/parser-base.h#newcode2356 src/parsing/parser-base.h:2356: case PropertyKind::kSpreadProperty: ...
3 years, 11 months ago (2017-01-05 22:39:14 UTC) #56
gsathya
https://codereview.chromium.org/2606833002/diff/240001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2606833002/diff/240001/src/objects.cc#newcode2136 src/objects.cc:2136: // 4a ii 2. Perform ? CreateDataProperty(target, nextKey, propValue). ...
3 years, 11 months ago (2017-01-05 22:56:29 UTC) #59
adamk
lgtm % test https://codereview.chromium.org/2606833002/diff/280001/test/mjsunit/harmony/object-spread-basic.js File test/mjsunit/harmony/object-spread-basic.js (right): https://codereview.chromium.org/2606833002/diff/280001/test/mjsunit/harmony/object-spread-basic.js#newcode105 test/mjsunit/harmony/object-spread-basic.js:105: assertEquals({}, y = { ...p }); ...
3 years, 11 months ago (2017-01-05 23:09:27 UTC) #60
gsathya
https://codereview.chromium.org/2606833002/diff/280001/test/mjsunit/harmony/object-spread-basic.js File test/mjsunit/harmony/object-spread-basic.js (right): https://codereview.chromium.org/2606833002/diff/280001/test/mjsunit/harmony/object-spread-basic.js#newcode105 test/mjsunit/harmony/object-spread-basic.js:105: assertEquals({}, y = { ...p }); On 2017/01/05 23:09:27, ...
3 years, 11 months ago (2017-01-05 23:17:28 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2606833002/300001
3 years, 11 months ago (2017-01-05 23:42:24 UTC) #68
commit-bot: I haz the power
3 years, 11 months ago (2017-01-05 23:44:34 UTC) #71
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/v8/v8/+/a40b7172fe7d255b4ce3055d4ca7cdbbb3f...

Powered by Google App Engine
This is Rietveld 408576698