Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 771863002: Add Array.prototype.includes (Closed)

Created:
4 years, 10 months ago by domenic (use chromium.org)
Modified:
4 years, 10 months ago
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Project:
v8
Visibility:
Public.

Description

Add Array.prototype.includes Requires adding a SameValueZero implementation. LOG=Y BUG=v8:3575 R=dslomov@chromium.org, arv@chromium.org TEST=added to test262

Patch Set 1 #

Total comments: 1

Patch Set 2 : Move to --harmony_array_includes flag and new file #

Patch Set 3 : Also update BUILD.gn #

Patch Set 4 : Add converted test262 tests #

Total comments: 9

Patch Set 5 : Fix comment wrapping to 80 cols #

Patch Set 6 : Added more tests and addressed nits #

Total comments: 2

Patch Set 7 : Fix Test262Error conversion to not go to MjsUnitAssertionError #

Patch Set 8 : Rebase on master #

Unified diffs Side-by-side diffs Delta from patch set Stats (+815 lines, -0 lines) Patch
M BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 5 6 7 3 chunks +4 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A src/harmony-array-includes.js View 1 1 chunk +61 lines, -0 lines 0 comments Download
M src/runtime.js View 1 2 3 4 5 6 7 1 chunk +9 lines, -0 lines 0 comments Download
A test/mjsunit/harmony/array-includes.js View 1 2 3 4 5 6 1 chunk +677 lines, -0 lines 0 comments Download
A test/mjsunit/harmony/array-includes-to-object-sloppy.js View 1 2 3 4 5 1 chunk +29 lines, -0 lines 0 comments Download
A test/mjsunit/harmony/array-includes-to-object-strict.js View 1 2 3 4 5 1 chunk +32 lines, -0 lines 0 comments Download
M tools/gyp/v8.gyp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 28 (5 generated)
domenic (use chromium.org)
For tests I would like guidance as to how we should proceed. The existing test262-format ...
4 years, 10 months ago (2014-12-02 01:18:58 UTC) #1
caitp (gmail)
On 2014/12/02 01:18:58, domenic wrote: > For tests I would like guidance as to how ...
4 years, 10 months ago (2014-12-02 02:03:00 UTC) #2
domenic (use chromium.org)
> It looks like the test262-es6 runner isn't able to pass in flags for unshipped ...
4 years, 10 months ago (2014-12-02 02:29:17 UTC) #3
Dmitry Lomov (no reviews)
On 2014/12/02 02:29:17, domenic wrote: > > It looks like the test262-es6 runner isn't able ...
4 years, 10 months ago (2014-12-02 12:22:40 UTC) #4
Dmitry Lomov (no reviews)
As a ES7 feature, this needs intent-to-implement (and separate flag) https://codereview.chromium.org/771863002/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/771863002/diff/1/src/harmony-array.js#newcode133 ...
4 years, 10 months ago (2014-12-02 12:25:26 UTC) #5
domenic (use chromium.org)
> I'd prefer tests in V8 repo, until we figure out our relationship with test262 ...
4 years, 10 months ago (2014-12-02 15:17:40 UTC) #6
arv (Not doing code reviews)
On 2014/12/02 at 15:17:40, d wrote: > > I'd prefer tests in V8 repo, until ...
4 years, 10 months ago (2014-12-02 15:19:02 UTC) #7
domenic (use chromium.org)
Arv clarified that we probably want a new flag but it would still start with ...
4 years, 10 months ago (2014-12-02 16:20:09 UTC) #8
arv (Not doing code reviews)
Impl looks good. Were you planning on moving the tests over or not? As is ...
4 years, 10 months ago (2014-12-02 16:26:48 UTC) #9
domenic (use chromium.org)
On 2014/12/02 16:26:48, arv wrote: > Impl looks good. Were you planning on moving the ...
4 years, 10 months ago (2014-12-02 16:39:04 UTC) #10
Dmitry Lomov (no reviews)
On 2014/12/02 16:39:04, domenic wrote: > On 2014/12/02 16:26:48, arv wrote: > > Impl looks ...
4 years, 10 months ago (2014-12-02 17:52:56 UTC) #11
domenic (use chromium.org)
On 2014/12/02 at 17:52:56, dslomov wrote: > For now, I'd prefer option 1. Tests uploaded!
4 years, 10 months ago (2014-12-04 20:42:08 UTC) #12
caitp (gmail)
These are really just nitpicky, but Other tests that are often present: Test that an ...
4 years, 10 months ago (2014-12-04 20:53:02 UTC) #14
arv (Not doing code reviews)
Maybe some tests on holey arrays with things on the prototype? Maybe some tests of ...
4 years, 10 months ago (2014-12-04 21:04:01 UTC) #15
domenic (use chromium.org)
caitp: > Test that an exception during a getter results propagates the exception Already there; ...
4 years, 10 months ago (2014-12-04 21:57:19 UTC) #16
Dmitry Lomov (no reviews)
lgtm, thanks for porting tests.
4 years, 10 months ago (2014-12-09 21:13:03 UTC) #17
arv (Not doing code reviews)
https://codereview.chromium.org/771863002/diff/100001/test/mjsunit/harmony/array-includes.js File test/mjsunit/harmony/array-includes.js (right): https://codereview.chromium.org/771863002/diff/100001/test/mjsunit/harmony/array-includes.js#newcode121 test/mjsunit/harmony/array-includes.js:121: }, MjsUnitAssertionError); I think assertUnreachable throws an MjsUnitAssertionError. Maybe ...
4 years, 10 months ago (2014-12-09 21:38:48 UTC) #18
domenic_domenicdenicola.com
Fixed to use dummy `function Test262Error() {}`s in the relevant tests; thanks arv!
4 years, 10 months ago (2014-12-09 22:15:56 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/771863002/120001
4 years, 10 months ago (2014-12-09 22:16:38 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1651) v8_win_rel on tryserver.v8 (http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel/builds/1867)
4 years, 10 months ago (2014-12-09 22:17:49 UTC) #24
domenic_domenicdenicola.com
Forgot to rebase!
4 years, 10 months ago (2014-12-09 22:41:07 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/771863002/140001
4 years, 10 months ago (2014-12-09 22:41:38 UTC) #27
commit-bot: I haz the power
4 years, 10 months ago (2014-12-10 08:58:14 UTC) #28
Message was sent while issue was closed.
Committed patchset #8 (id:140001)

Powered by Google App Engine
This is Rietveld 408576698