Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
579973002:
Add Array.prototype.contains (Closed)
|
Created:
6 years, 3 months ago by domenic Modified:
6 years ago Reviewers:
caitp (gmail), Dmitry Lomov (no reviews), arv (Not doing code reviews), rossberg, mathias CC:
v8-dev Project:
v8 Visibility:
Public. |
DescriptionAdd Array.prototype.contains
Requires adding ToLength and SameValueZero implementations.
LOG=Y
BUG=none
R=rossberg@chromium.org, arv@chromium.org
TEST=added to test262
Patch Set 1 #
Total comments: 16
Patch Set 2 : Update for code review comments #Patch Set 3 : Add v8 bug to commit message #
Total comments: 1
Patch Set 4 : Using %FunctionSetLength instead of %_Arguments #Patch Set 5 : Use `array` instead of `O` #Messages
Total messages: 24 (3 generated)
On 2014/09/18 01:16:23, domenic wrote: On tests: test262 tests still need to be merged (https://github.com/tc39/test262/pull/95) but once they are I can update the CL to include the new test262 SHA. The tests are pretty comprehensive and all passing locally. But I was hoping to get the code up for review ahead of time. In general: I have no idea what I am doing and would love feedback on code, CL etiquette, or anything else relevant.
This is my first CL; feedback much appreciated. I don't even know if I'm supposed to be pressing this "Publish+Mail Comments" button, but some slide deck told me to.
caitpotter88@gmail.com changed reviewers: + caitpotter88@gmail.com
https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode129 src/harmony-array.js:129: function ArrayContains(searchElement /*, fromIndex */) { // length == 1 arv@ has suggested using `%FunctionSetLength(function, length);` to get the length right instead, so that we don't have tp use weirdness like %_Arguments()
On 2014/09/18 01:28:31, caitp wrote: > https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js > File src/harmony-array.js (right): > > https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode129 > src/harmony-array.js:129: function ArrayContains(searchElement /*, fromIndex */) > { // length == 1 > arv@ has suggested using `%FunctionSetLength(function, length);` to get the > length right instead, so that we don't have tp use weirdness like %_Arguments() Yang overruled that in the past in favor of the currently used technique. Domenic, nice work! Don’t forget to increment `EXPECTED_BUILTINS_COUNT` in `tools/generate-runtime-tests.py`.
mathias@qiwi.be changed reviewers: + mathias@qiwi.be
https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode129 src/harmony-array.js:129: function ArrayContains(searchElement /*, fromIndex */) { // length == 1 Needs an extra space before `//`.
https://codereview.chromium.org/579973002/diff/1/src/runtime.js File src/runtime.js (right): https://codereview.chromium.org/579973002/diff/1/src/runtime.js#newcode581 src/runtime.js:581: if (%_IsSmi(x) && x > 0) return x; // not >= since -0 >= +0 Same here.
dslomov@chromium.org changed reviewers: + dslomov@chromium.org
Cool. Few comments. As this is a new unapproved feature, this needs intent-to-implement mail. I guess this is the first time when somebody add test262 tests first and patch later, but I think we still want some tests in our tree (under test/mjsunit/harmony) Code comemnts below: https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode129 src/harmony-array.js:129: function ArrayContains(searchElement /*, fromIndex */) { // length == 1 Needs COERCIBLE check. (No strong preference on how you set the length of function/access arguments) https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode133 src/harmony-array.js:133: if (len === 0) { Do you need this check? The below will work just fine for zero length. https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode139 src/harmony-array.js:139: if (n >= len) { Do you need this check? 'while' below has a check already. https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode147 src/harmony-array.js:147: k = len - MathAbs(n); Simply len + n? https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode155 src/harmony-array.js:155: if (SameValueZero(searchElement, elementK) === true) { === true? Why not "if (SameValueZero(...))" https://codereview.chromium.org/579973002/diff/1/src/runtime.js File src/runtime.js (right): https://codereview.chromium.org/579973002/diff/1/src/runtime.js#newcode581 src/runtime.js:581: if (%_IsSmi(x) && x > 0) return x; // not >= since -0 >= +0 if %_IsSmi(x) then x can only be +0 (-0 is not Smi). Feel free to do: if (%_IsSmi(x)) return (x > 0 ? x : 0); All Smis are safe
Re CL description: either remove LOG=Y and BUG=None, or add a tracking bug to V8 tracker.
Can you remind me what stage this proposal is currently in? I agree with Dmitry that there needs to be an intent mail and appropriate discussion, especially since this sets a precedent for our future handling of ES7 features. I also think that the implementation needs to go under a different flag than any ES6 functionality, since we need to be able to activate and ship one without the other. (For the same reason, it does not make sense to push tests for it into test262 at this time. At least not until test262 has a way to distinguish language versions.)
On 2014/09/18 at 06:11:16, mathias wrote: > On 2014/09/18 01:28:31, caitp wrote: > > https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js > > File src/harmony-array.js (right): > > > > https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode129 > > src/harmony-array.js:129: function ArrayContains(searchElement /*, fromIndex */) > > { // length == 1 > > arv@ has suggested using `%FunctionSetLength(function, length);` to get the > > length right instead, so that we don't have tp use weirdness like %_Arguments() > > Yang overruled that in the past in favor of the currently used technique. That was the opposite case. In this case I find it better to use %FunctionSetLength since using %_Arguments(1) is not as clear. > Domenic, nice work! Don’t forget to increment `EXPECTED_BUILTINS_COUNT` in `tools/generate-runtime-tests.py`. The runtime tests have been removed.
> As this is a new unapproved feature, this needs intent-to-implement mail. Will send > I guess this is the first time when somebody add test262 tests first and patch later, but I think we still want some tests in our tree (under test/mjsunit/harmony) I would really like us to break out of the pattern of creating our own tests separate from the vendor-independent ones. I am hoping we can pioneer that process here. I am also going to be investigating improvements to test262 to make writing tests there as easy as mjsunit ones (e.g. adding assert helpers). Perhaps it would help to have an "unsubmitted" subfolder of the test262 tests that we can check in so as to decouple ourselves from the merging process of test262? That is what Mozilla will likely be doing, it seems: https://groups.google.com/forum/#!topic/mozilla.dev.tech.js-engine.internals/... > Can you remind me what stage this proposal is currently in? Stage 1. Advancement to stage 2 requires experimental implementations, which is the point of this patch (as well as the Mozilla one) :) > I agree with Dmitry that there needs to be an intent mail and appropriate > discussion, especially since this sets a precedent for our future handling of > ES7 features. I also think that the implementation needs to go under a different > flag than any ES6 functionality, since we need to be able to activate and ship > one without the other. Happy to have that discussion. That is pretty much the point of Array.prototype.contains after all. > (For the same reason, it does not make sense to push tests for it into test262 > at this time. At least not until test262 has a way to distinguish language > versions.) test262 is generally on-board with the idea of decoupling what version of the Ecma standard a feature is in from its presence in the test suite, so it will likely end up there since it has approval.
https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode129 src/harmony-array.js:129: function ArrayContains(searchElement /*, fromIndex */) { // length == 1 On 2014/09/18 06:26:45, Dmitry Lomov (chromium) wrote: > Needs COERCIBLE check. (No strong preference on how you set the length of > function/access arguments) There is no COERCIBLE check in the spec for any of the array methods these days. Its presence in V8 seems to be a separate bug? https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode133 src/harmony-array.js:133: if (len === 0) { On 2014/09/18 06:26:45, Dmitry Lomov (chromium) wrote: > Do you need this check? The below will work just fine for zero length. Yes, as otherwise we will trigger ToInteger on fromIndex, which has observable consequences for e.g. fromIndex = { valueOf() { /* side effects */ } } https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode139 src/harmony-array.js:139: if (n >= len) { On 2014/09/18 06:26:45, Dmitry Lomov (chromium) wrote: > Do you need this check? 'while' below has a check already. Removed, tests still pass. https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode147 src/harmony-array.js:147: k = len - MathAbs(n); On 2014/09/18 06:26:45, Dmitry Lomov (chromium) wrote: > Simply len + n? Weird, I wonder why the spec used abs for indexOf (which I copied). Fixing. https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode155 src/harmony-array.js:155: if (SameValueZero(searchElement, elementK) === true) { On 2014/09/18 06:26:45, Dmitry Lomov (chromium) wrote: > === true? Why not "if (SameValueZero(...))" Just going for spec-verbatimness. Will change.
Another proplem is that we are not running the test262 tests by default. https://codereview.chromium.org/579973002/diff/40001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/579973002/diff/40001/src/harmony-array.js#new... src/harmony-array.js:130: var O = ToObject(this); Rename this to object or array.
On 2014/09/18 15:15:50, domenic wrote: > > As this is a new unapproved feature, this needs intent-to-implement mail. > > Will send > > > I guess this is the first time when somebody add test262 tests first and patch > later, but I think we still want some tests in our tree (under > test/mjsunit/harmony) > > I would really like us to break out of the pattern of creating our own tests > separate from the vendor-independent ones. I am hoping we can pioneer that > process here. I am also going to be investigating improvements to test262 to > make writing tests there as easy as mjsunit ones (e.g. adding assert helpers). > That is worthwhile (and I think that would be great in ideal world) but it suffers from a couple of issues: 1. Currently, we do not run test262 tests on dev machines during development, relying on mjsunit for smoke testing (we run test262 on waterfall though) 2. Without seeing the tests, it is hard to evaluate the coverage this CL has.
One more comment: since Array.prototype.contains is not a ES6 feature, it should probably be under a flag separate from --harmony-arrays. But I would prefer to have that discussion on intent-to-implement thread. https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode129 src/harmony-array.js:129: function ArrayContains(searchElement /*, fromIndex */) { // length == 1 On 2014/09/18 15:16:13, domenic wrote: > On 2014/09/18 06:26:45, Dmitry Lomov (chromium) wrote: > > Needs COERCIBLE check. (No strong preference on how you set the length of > > function/access arguments) > > There is no COERCIBLE check in the spec for any of the array methods these days. > Its presence in V8 seems to be a separate bug? Hmm, I see. Could you file a bug? https://codereview.chromium.org/579973002/diff/1/src/harmony-array.js#newcode133 src/harmony-array.js:133: if (len === 0) { On 2014/09/18 15:16:13, domenic wrote: > On 2014/09/18 06:26:45, Dmitry Lomov (chromium) wrote: > > Do you need this check? The below will work just fine for zero length. > > Yes, as otherwise we will trigger ToInteger on fromIndex, which has observable > consequences for e.g. fromIndex = { valueOf() { /* side effects */ } } Got it.
On 2014/09/18 15:15:50, domenic wrote: > > (For the same reason, it does not make sense to push tests for it into test262 > > at this time. At least not until test262 has a way to distinguish language > > versions.) > > test262 is generally on-board with the idea of decoupling what version of the > Ecma standard a feature is in from its presence in the test suite, so it will > likely end up there since it has approval. Okay, that's probably a good topic to discuss next week. We should definitely have a way to include these tests into test262 early on, but I think that only makes sense with a staging process that mirrors the proposal stages.
On 2014/09/18 15:34:54, Dmitry Lomov (chromium) wrote: > That is worthwhile (and I think that would be great in ideal world) but it > suffers from a couple of issues: > 1. Currently, we do not run test262 tests on dev machines during development, > relying on mjsunit for smoke testing (we run test262 on waterfall though) Ah, I see. Interested to figure out how we can fix this. > 2. Without seeing the tests, it is hard to evaluate the coverage this CL has. Tests are at https://github.com/tc39/test262/pull/95/files; the coverage is pretty ridiculously exhaustive, if I may say so myself ;) > Hmm, I see. Could you file a bug? Filed https://code.google.com/p/v8/issues/detail?id=3577
To my mind the next steps are as follows: - Continue discussion of flags and such on the intent to implement thread (https://groups.google.com/forum/#!topic/v8-dev/Th-67YERn1I) - Add a way to commit test262 tests to V8 that are not merged into the official test262 test suite yet. I envision e.g. a "test262-pending" directory or similar. - Add the running of a subset of test262 tests to the developer smoke test, so that we can have Array.prototype.contains tests committed to test262-pending and run by developers. Once the A.p.c tests are merged into test262 proper, we will still want to run them---but not the rest of the test262 suite---as part of the development process. Over time, as we add test262 tests instead of mjsunit tests, we can expand the whitelisted set of tests to run. Arv, Andreas and I can discuss the particulars of how to go about this in person at TC39 this week.
Superceded by https://codereview.chromium.org/771863002 |
