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

Issue 364853009: Implement ES6 Array.of() (Closed)

Created:
6 years, 5 months ago by Diego Pino
Modified:
6 years, 4 months ago
CC:
v8-dev
Base URL:
https://github.com/v8/v8@master
Project:
v8
Visibility:
Public.

Description

Implement ES6 Array.of() BUG=v8:3427 LOG=Y

Patch Set 1 #

Total comments: 3

Patch Set 2 : Removed DONT_ENUM and recoded IsConstructor to handle function proxies #

Patch Set 3 : Removed implementation of IsConstructor and used IS_SPEC_FUNCTION instead #

Patch Set 4 : Added comment 'TODO: Implement IsConstructor' #

Total comments: 3

Patch Set 5 : Fixed nits. All tests now in one file. Added implementation of AddElement. #

Patch Set 6 : Renamed array-of-tests.js to array.of.js. Moved test file to harmony/ folder. #

Patch Set 7 : Initialize index #

Patch Set 8 : Fix indentation in harmony-array.js. Add comments to AddElement. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+213 lines, -3 lines) Patch
M src/harmony-array.js View 1 2 3 4 5 6 7 1 chunk +18 lines, -0 lines 0 comments Download
M src/runtime.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M src/runtime.cc View 1 2 3 4 5 6 7 1 chunk +27 lines, -0 lines 0 comments Download
A test/mjsunit/harmony/array-of.js View 1 2 3 4 5 1 chunk +164 lines, -0 lines 0 comments Download
M tools/generate-runtime-tests.py View 1 2 3 4 5 6 7 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 28 (0 generated)
Diego Pino
PTAL
6 years, 5 months ago (2014-07-07 16:37:39 UTC) #1
caitp (gmail)
Just one comment, it turns out --- https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createdatapropertyorthrow (which defers to https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createdataproperty) says that the ...
6 years, 5 months ago (2014-07-07 16:52:09 UTC) #2
Diego Pino
On 2014/07/07 16:52:09, caitp wrote: > Just one comment, it turns out --- > https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createdatapropertyorthrow ...
6 years, 5 months ago (2014-07-07 17:23:44 UTC) #3
caitp (gmail)
On 2014/07/07 17:23:44, Diego Pino wrote: > On 2014/07/07 16:52:09, caitp wrote: > > Just ...
6 years, 5 months ago (2014-07-07 17:30:36 UTC) #4
rossberg
https://codereview.chromium.org/364853009/diff/1/src/runtime.cc File src/runtime.cc (right): https://codereview.chromium.org/364853009/diff/1/src/runtime.cc#newcode7807 src/runtime.cc:7807: RUNTIME_FUNCTION(Runtime_IsConstructor) { Drive-by-comment: Unfortunately, I don't think this function ...
6 years, 5 months ago (2014-07-08 08:39:17 UTC) #5
rossberg
https://codereview.chromium.org/364853009/diff/1/src/runtime.cc File src/runtime.cc (right): https://codereview.chromium.org/364853009/diff/1/src/runtime.cc#newcode7807 src/runtime.cc:7807: RUNTIME_FUNCTION(Runtime_IsConstructor) { On 2014/07/08 08:39:17, rossberg wrote: > Drive-by-comment: ...
6 years, 5 months ago (2014-07-08 09:59:09 UTC) #6
caitp (gmail)
So just a question, are we blocked on filling out the rest of the harmony ...
6 years, 5 months ago (2014-07-14 18:54:33 UTC) #7
rossberg
On 2014/07/14 18:54:33, caitp wrote: > So just a question, are we blocked on filling ...
6 years, 5 months ago (2014-07-16 09:52:15 UTC) #8
Diego Pino
On 2014/07/16 09:52:15, rossberg wrote: > No, no need to block on that. Just use ...
6 years, 5 months ago (2014-07-21 11:23:09 UTC) #9
rossberg
Can you please merge all the tests into one file? https://codereview.chromium.org/364853009/diff/60001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/364853009/diff/60001/src/harmony-array.js#newcode132 ...
6 years, 5 months ago (2014-07-21 16:07:07 UTC) #10
Diego Pino
On 2014/07/21 16:07:07, rossberg wrote: > Can you please merge all the tests into one ...
6 years, 5 months ago (2014-07-23 11:47:29 UTC) #11
rossberg
LGTM, with one nit: please rename array-of-tests.js to just array-of.js, and move it to the ...
6 years, 5 months ago (2014-07-23 12:19:14 UTC) #12
Diego Pino
I think the patch is ready to land now. All try bots are green except ...
6 years, 4 months ago (2014-07-29 15:30:42 UTC) #13
caitp (gmail)
I'm not an owner, so take this with a grain of salt: https://codereview.chromium.org/364853009/diff/260001/src/harmony-array.js File src/harmony-array.js ...
6 years, 4 months ago (2014-08-05 02:09:07 UTC) #14
caitp (gmail)
For what it's worth, if AddElement is in some way a useful implementation of CreateDataPropertyOrThrow, ...
6 years, 4 months ago (2014-08-05 02:11:14 UTC) #15
Diego Pino
Thanks caitp for your comments, I think they are very valid and I also have ...
6 years, 4 months ago (2014-08-08 12:22:50 UTC) #16
Diego Pino
On 2014/08/05 02:11:14, caitp wrote: > For what it's worth, if AddElement is in some ...
6 years, 4 months ago (2014-08-08 12:29:10 UTC) #17
rossberg
CreateDataPropertyOrThrow is new in the ES6 spec, which explains why it hasn't occurred in V8 ...
6 years, 4 months ago (2014-08-08 12:53:38 UTC) #18
rossberg
On 2014/08/08 12:53:38, rossberg wrote: > For the time being, CreateDataProperty should translate to AddProperty/Element. ...
6 years, 4 months ago (2014-08-08 12:54:31 UTC) #19
Diego Pino
Thanks for the clarifications. So I'm going to leave the patch as it is and ...
6 years, 4 months ago (2014-08-08 20:57:04 UTC) #20
Diego Pino
The CQ bit was checked by dpino@igalia.com
6 years, 4 months ago (2014-08-18 14:51:36 UTC) #21
Diego Pino
The CQ bit was unchecked by dpino@igalia.com
6 years, 4 months ago (2014-08-18 14:51:36 UTC) #22
Diego Pino
All trybots are green now.
6 years, 4 months ago (2014-08-18 14:53:23 UTC) #23
Diego Pino
The CQ bit was checked by dpino@igalia.com
6 years, 4 months ago (2014-08-18 16:38:49 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://v8-status.appspot.com/cq/dpino@igalia.com/364853009/340001
6 years, 4 months ago (2014-08-18 16:39:24 UTC) #25
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-18 16:39:25 UTC) #26
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...
6 years, 4 months ago (2014-08-18 16:39:26 UTC) #27
rossberg
6 years, 4 months ago (2014-08-19 11:11:03 UTC) #28
Note that V8 does not have a commit queue, so the CQ bit isn't operational. I
will land for you. (Sorry for the delay, I was OOO last week.)

Powered by Google App Engine
This is Rietveld 408576698