Bug 130966 - Implement Array.prototype.find()
Summary: Implement Array.prototype.find()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords:
: 131701 131702 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-03-31 04:21 PDT by Antoine Quint
Modified: 2014-05-07 12:32 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.75 KB, patch)
2014-04-01 02:30 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (15.62 KB, patch)
2014-04-24 03:29 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (19.66 KB, patch)
2014-04-24 03:41 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (24.44 KB, patch)
2014-04-25 00:48 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2014-03-31 04:21:04 PDT
I've recently stumbled upon the Array.prototype.find() method proposed in Harmony while looking for a way to establish which of the values contained in an array was found in a string first. Mozilla implements this method, and it would be nice for JSC to implement it too.
Comment 1 Antoine Quint 2014-04-01 02:30:39 PDT
Created attachment 228259 [details]
Patch
Comment 2 Oliver Hunt 2014-04-01 10:31:27 PDT
Comment on attachment 228259 [details]
Patch

Please add tests for 0 vs undefined vs false vs null vs "" -- we want tests that cover the specific kind of equality.  Also perf numbers relative to v8 and SM would be good.

Test cases for array modification during search, 

Both test cases need to be bigger and should be using the shouldBe() shouldBeTrue(), etc functions.  In general js/regress is for performancy tests where we're trying to measure speed and jit behaviour.  and they use throw on incorrect result as they need to ensure that the jit's don't DCE the entire test.

So r-, you need more tests.  I suspect the filter, etc tests in js/ should give a good example of expected behaviour.
Comment 3 Antoine Quint 2014-04-02 01:17:24 PDT
Thanks for the feedback, I had a feeling I wasn't writing the right kind of tests. V8 doesn't implement those methods.
Comment 4 Antoine Quint 2014-04-24 03:29:05 PDT
Created attachment 230067 [details]
Patch
Comment 5 Antoine Quint 2014-04-24 03:41:52 PDT
Created attachment 230068 [details]
Patch
Comment 6 Darin Adler 2014-04-24 09:18:42 PDT
Comment on attachment 230068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230068&action=review

> Source/JavaScriptCore/builtins/Array.prototype.js:195
> +        throw new @TypeError("Array.prototype.find requires that |this| not be null");

I don’t see any test coverage of this.

> Source/JavaScriptCore/builtins/Array.prototype.js:198
> +        throw new @TypeError("Array.prototype.find requires that |this| not be undefined");

I don’t see any test coverage of this.

> Source/JavaScriptCore/builtins/Array.prototype.js:204
> +        throw new @TypeError("Array.prototype.find callback must be a function");

I don’t see any test coverage of this.

> Source/JavaScriptCore/builtins/Array.prototype.js:217
> +        throw new @TypeError("Array.prototype.findIndex requires that |this| not be null");

I don’t see any test coverage of this.

> Source/JavaScriptCore/builtins/Array.prototype.js:220
> +        throw new @TypeError("Array.prototype.findIndex requires that |this| not be undefined");

I don’t see any test coverage of this.

> Source/JavaScriptCore/builtins/Array.prototype.js:226
> +        throw new @TypeError("Array.prototype.findIndex callback must be a function");

I don’t see any test coverage of this.
Comment 7 Oliver Hunt 2014-04-24 09:59:48 PDT
Comment on attachment 230068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230068&action=review

r=me, but add edge case tests before landing

>> Source/JavaScriptCore/builtins/Array.prototype.js:198
>> +        throw new @TypeError("Array.prototype.find requires that |this| not be undefined");
> 
> I don’t see any test coverage of this.

Add .find.call(...) tests

>> Source/JavaScriptCore/builtins/Array.prototype.js:204
>> +        throw new @TypeError("Array.prototype.find callback must be a function");
> 
> I don’t see any test coverage of this.

coverage test

>> Source/JavaScriptCore/builtins/Array.prototype.js:226
>> +        throw new @TypeError("Array.prototype.findIndex callback must be a function");
> 
> I don’t see any test coverage of this.

ditto
Comment 8 Oliver Hunt 2014-04-24 10:00:17 PDT
Oh, also add tests for arrays with holes
Comment 9 Antoine Quint 2014-04-25 00:48:34 PDT
Created attachment 230148 [details]
Patch for landing
Comment 10 Antoine Quint 2014-04-25 00:53:38 PDT
Patch for landing should have the exceptions and array with holes covered. Thanks for the review!
Comment 11 WebKit Commit Bot 2014-04-25 01:26:27 PDT
Comment on attachment 230148 [details]
Patch for landing

Clearing flags on attachment: 230148

Committed r167797: <http://trac.webkit.org/changeset/167797>
Comment 12 WebKit Commit Bot 2014-04-25 01:26:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Joseph Pecoraro 2014-04-25 10:11:40 PDT
Comment on attachment 230148 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=230148&action=review

> Source/JavaScriptCore/builtins/Array.prototype.js:210
> +    for (var i = 0; i < length; i++) {
> +        if (callback.@call(thisArg, array[i], i, array))
> +            return array[i];
> +    }

Should we only call the callback if "i in array" for sparse arrays?

<http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.find>

Seems to indicate it only proceeds to calling the callback if the "i" was present:

. Repeat, while k < len
. Let Pk be ToString(k).
. Let kPresent be HasProperty(O, Pk).
. ReturnIfAbrupt(kPresent).
. If kPresent is true, then
.   . Let kValue be Get(O, Pk).
.   . ReturnIfAbrupt(kValue).
.   . Let testResult be the result of calling the [[Call]] internal method of predicate with T as thisArgument and a List containing kValue, k, and O as argumentsList.

I'm guessing a test for this would be:

    arrayWithHoles = [];
    arrayWithHoles[10] = 0;
    arrayWithHoles[20] = null;
    arrayWithHoles[30] = false;
    arrayWithHoles[40] = "";
    arrayWithHoles.findIndex(passUndefined); // -1
    arrayWithHoles[50] = undefined;
    arrayWithHoles.findIndex(passUndefined); // 50

I could be wrong though. I didn't test in Firefox to see what they do.
Comment 14 Antoine Quint 2014-05-07 10:51:46 PDT
*** Bug 131698 has been marked as a duplicate of this bug. ***
Comment 15 Antoine Quint 2014-05-07 10:51:58 PDT
*** Bug 131702 has been marked as a duplicate of this bug. ***
Comment 16 Antoine Quint 2014-05-07 10:52:55 PDT
*** Bug 131701 has been marked as a duplicate of this bug. ***
Comment 17 Joseph Pecoraro 2014-05-07 12:32:23 PDT
> I could be wrong though. I didn't test in Firefox to see what they do.

Just tested in Firefox, and they do -1 and 50. We do 0 and 0. Filed:
<https://webkit.org/b/132658> Array.prototype.find and findIndex should skip holes