Bug 159385 – ECMAScript 2016: %TypedArray%.prototype.includes implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

Implement the %TypedArray%.prototype.includes ( searchElement [ , fromIndex ] ) following the ECMAScript 2016 draft version available in https://tc39.github.io/ecma262/2016/#sec-%typedarray%.prototype.includes

[Caio Lima]

Created attachment 282639 [details]
RFC Patch

[WebKit Commit Bot]

Attachment 282639 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Comment on attachment 282639 [details]
RFC Patch

This Patch contains the implementation of JavaScript version and also the C++ version. I've made a benchmark with this code:

let a = new Float64Array(100);

a.fill(1);

for (let i = 0; i < 100000000; i++) {
   a.includes(0);
}

The idea is reproduce the worst case, since it is going to compare with all array elements. These are the configurations of runs:

----------C++ Implementation----------
Caios-MacBook-Pro:js-tests caiolima$ JSC_useJIT=false time run-jsc includes-benchmark.js 
Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/caiolima/open_projects/WebKit/WebKitBuild/Release /Users/caiolima/open_projects/WebKit/WebKitBuild/Release/jsc includes-benchmark.js
       16.36 real        16.04 user         0.16 sys
Caios-MacBook-Pro:js-tests caiolima$ JSC_useJIT=true time run-jsc includes-benchmark.js 
Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/caiolima/open_projects/WebKit/WebKitBuild/Release /Users/caiolima/open_projects/WebKit/WebKitBuild/Release/jsc includes-benchmark.js
       13.71 real        13.40 user         0.16 sys

----------JS Wraper Implementation----------
Caios-MacBook-Pro:js-tests caiolima$ JSC_useJIT=false time run-jsc includes-benchmark.js 
Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/caiolima/open_projects/WebKit/WebKitBuild/Release /Users/caiolima/open_projects/WebKit/WebKitBuild/Release/jsc includes-benchmark.js
      345.25 real       343.33 user         1.24 sys
Caios-MacBook-Pro:js-tests caiolima$ 
Caios-MacBook-Pro:js-tests caiolima$ JSC_useJIT=true time run-jsc includes-benchmark.js 
Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/caiolima/open_projects/WebKit/WebKitBuild/Release /Users/caiolima/open_projects/WebKit/WebKitBuild/Release/jsc includes-benchmark.js
       12.54 real        12.22 user         0.23 sys

What I think is the C++ implementation has better LLInt - JIT balance, since it performs well with and without JIT. However, When the javascript wrapper is JITed,it performs better than the C++ version JITed. What do you guys think about it. Also, I am open to new Benchmark ideas.

[Benjamin Poulain]

Comment on attachment 282639 [details]
RFC Patch

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

An implementation of TypedArray's includes, that's awesome!

Some comments bellow:

> Source/JavaScriptCore/ChangeLog:10
> +				%TypedArray%.prototype.includes following spec 22.2.3.14
> +				https://tc39.github.io/ecma262/2016/#sec-%typedarray%.prototype.includes

Be careful to use spaces for indentation.
We avoid mixing tabs and spaces, the rule is to always use spaces.

> Source/JavaScriptCore/ChangeLog:19
> +

Don't forget to add test coverage too!

You should have tests in LayoutTests/js that try various use cases and verify the correctness.
There should also be a test getting the property descriptor and checking its properties (name, enumerable, etc).

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:355
> +        fromIndex = arguments[1] | 0;

This looks suspicious to me.

The specs says:
    Let n be ? ToInteger(fromIndex). (If fromIndex is undefined, this step produces the value 0.)

ToInteger() produces integer for values > INT_MAX.

x | 0 is a ToInt32() on x. The bounds are [INT_MIN, INT_MAX]

Looking at Array.prototype, I suspect we have a bug there.

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:370
> +        if (searchElement === currentElement || (searchElement !== searchElement && currentElement !== currentElement))

The loop would benefit from being specialized on searchElement !== searchElement.

if (searchElement === searchElement) {
    ... Loop searching for searchElement
} else {
    ... Loop searching for NaN
}

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222
> +        if (array[index] == target)

Isn't that missing the NaN case?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:223
> +            return JSValue::encode(JSValue(JSValue::JSTrue));

You can also use jsBoolean(true/false) to produce a boolean JSValue.

[Caio Lima]

Created attachment 282681 [details]
Patch

[Caio Lima]

(In reply to comment #4)
> Comment on attachment 282639 [details]
> RFC Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282639&action=review
> 
> An implementation of TypedArray's includes, that's awesome!
> 
> Some comments bellow:
> 
> > Source/JavaScriptCore/ChangeLog:10
> > +				%TypedArray%.prototype.includes following spec 22.2.3.14
> > +				https://tc39.github.io/ecma262/2016/#sec-%typedarray%.prototype.includes
> 
> Be careful to use spaces for indentation.
> We avoid mixing tabs and spaces, the rule is to always use spaces.

Sorry, my vim is not configured correctly. Thanks for point it =).

> > Source/JavaScriptCore/ChangeLog:19
> > +
> 
> Don't forget to add test coverage too!
> 
> You should have tests in LayoutTests/js that try various use cases and
> verify the correctness.
> There should also be a test getting the property descriptor and checking its
> properties (name, enumerable, etc).

I added coverage, but I didn't consider cases about property descriptor. I am going to update it soon.

> > Source/JavaScriptCore/builtins/TypedArrayPrototype.js:355
> > +        fromIndex = arguments[1] | 0;
> 
> This looks suspicious to me.
> 
> The specs says:
>     Let n be ? ToInteger(fromIndex). (If fromIndex is undefined, this step
> produces the value 0.)
> 
> ToInteger() produces integer for values > INT_MAX.
> 
> x | 0 is a ToInt32() on x. The bounds are [INT_MIN, INT_MAX]
> 
> Looking at Array.prototype, I suspect we have a bug there.

I agree with you. I double checked the spec and confirm it. However, I am not sure if it is possible create an Array with length > INT_MAX. I need to check it.

> > Source/JavaScriptCore/builtins/TypedArrayPrototype.js:370
> > +        if (searchElement === currentElement || (searchElement !== searchElement && currentElement !== currentElement))
> 
> The loop would benefit from being specialized on searchElement !==
> searchElement.
> 
> if (searchElement === searchElement) {
>     ... Loop searching for searchElement
> } else {
>     ... Loop searching for NaN
> }

Nice! However I decided pick the C++ version in the new Patch.

> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222
> > +        if (array[index] == target)
> 
> Isn't that missing the NaN case?

Now I am considering it. However I have a doubt with suspicious cases:

1. If the searchElement is an Object or a String? In IntXArray, they are converted to 0, so "new UInt8Array([0, 1, 2]).includes("abc"); // is true". It is not just the case of %TypedArray%.prototype.includes, but also %TypedArray%.prototype.indexOf. I didn't find any information about it in the Spec. I checked v8 implementation and they return false to these cases. IMHO, I think it is the best result, since "new UInt8Array([0, 1, 2]).includes("abc");" returning "true" is a potential unpredictable bug source.

> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:223
> > +            return JSValue::encode(JSValue(JSValue::JSTrue));
> 
> You can also use jsBoolean(true/false) to produce a boolean JSValue.

Thank you for this Tip!

[Build Bot]

Comment on attachment 282681 [details]
Patch

Attachment 282681 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1622898

New failing tests:
js/regress/typed-array-includes.html

[Build Bot]

Created attachment 282685 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282681 [details]
Patch

Attachment 282681 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1622899

New failing tests:
js/regress/typed-array-includes.html

[Build Bot]

Created attachment 282686 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282681 [details]
Patch

Attachment 282681 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1622900

New failing tests:
js/regress/typed-array-includes.html

[Build Bot]

Created attachment 282688 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282681 [details]
Patch

Attachment 282681 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1622901

New failing tests:
js/regress/typed-array-includes.html

[Build Bot]

Created attachment 282689 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Caio Lima]

Created attachment 282691 [details]
Patch

[Benjamin Poulain]

> > > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222
> > > +        if (array[index] == target)
> > 
> > Isn't that missing the NaN case?
> 
> Now I am considering it. However I have a doubt with suspicious cases:
> 
> 1. If the searchElement is an Object or a String? In IntXArray, they are
> converted to 0, so "new UInt8Array([0, 1, 2]).includes("abc"); // is true".
> It is not just the case of %TypedArray%.prototype.includes, but also
> %TypedArray%.prototype.indexOf. I didn't find any information about it in
> the Spec. I checked v8 implementation and they return false to these cases.
> IMHO, I think it is the best result, since "new UInt8Array([0, 1,
> 2]).includes("abc");" returning "true" is a potential unpredictable bug
> source.

You are right, 
    new UInt8Array([0, 1, 2]).includes("abc")
and
    new UInt8Array([0, 1, 2]).indexOf("abc")
should return false. Converting the string to a number makes no sense.

The spec does not say to convert the input to a number:
-https://tc39.github.io/ecma262/#sec-%typedarray%.prototype.indexof
-https://tc39.github.io/ecma262/#sec-array.prototype.indexof

You have discovered a really stupid bug in the existing indexOf!

[Caio Lima]

Created attachment 282739 [details]
Patch

[Caio Lima]

Created attachment 282752 [details]
Patch

[Benjamin Poulain]

Comment on attachment 282752 [details]
Patch

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

Second round of review:

> Source/JavaScriptCore/ChangeLog:6
> +        Reviewed by Benjamin Poulain.

You don't need to modify this field.

The tool that lands the page (webkit-patch) replaces "NOBODY (OOPS!)" by the name of the reviewer found on bugzilla.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:211
> +    TypedArrayType arrayType = exec->thisValue().getObject()->classInfo()->typedArrayStorageType;

This is less efficient than it should.

You are getting the array type at runtime.
This condition can be fully determined at compile time from the template argument ViewClass.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:216
> +    if (!valueToFind.isNumber() && !valueToFind.isDouble())

Isn't "isNumber()" a superset of "isDouble()"?

valueToFind.isNumber() false implies valueToFind.isDouble() false.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222
> +    typename ViewClass::ElementType target = ViewClass::toAdaptorNativeFromValue(exec, valueToFind);

I don't think that works for includes.

For example, Uint8ClampedAdaptor will clamp positive values to 255, that's not what we need here.
IntegralTypedArrayAdaptor does ToInt32 on the input, that's not what we need either.

Your test should have extensive coverage of those crazy cases.

----

What you probably need is a new Adaptor function that returns a value in the right type or an error.

For example, if you pass a double to an integer type array, it returns the integer if it fits in the array type or fail.
E.g.:
    -The double 256.0 works for int16, fails for int8
    -The double 254.1 fails for all integers.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:226
> +    if (std::isnan(target)) {

This can be eliminated at compile time with the test described above for ViewClass.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:236
> +            if (JSValue::strictEqual(exec, currentValue, valueToFind))

I think it is not a good idea to use strictEqual() for performance.
The type of the Array is known, and the type of the valueToFind can be converted to that type.

> LayoutTests/ChangeLog:13
> +        * js/regress/script-tests/typed-array-includes.js: Added.

Any reason this is in js/regress and not in js/

> LayoutTests/js/regress/script-tests/typed-array-includes.js:1
> +function assert(a) {

You can use shouldBeTrue(), available in js-test-pre.js.

> LayoutTests/js/regress/script-tests/typed-array-includes.js:22
> +    assert(!tArray.includes("abc"));

It is worth testing all the types: undefined, null, empty string, regular object.

> LayoutTests/js/regress/script-tests/typed-array-includes.js:96
> +

Some things worth checking too:
Object.getPrototypeOf(TypedArray).prototype.name === "includes"
Object.getPrototypeOf(TypedArray).prototype.length === 1

[Keith Miller]

Comment on attachment 282752 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:211
>> +    TypedArrayType arrayType = exec->thisValue().getObject()->classInfo()->typedArrayStorageType;
> 
> This is less efficient than it should.
> 
> You are getting the array type at runtime.
> This condition can be fully determined at compile time from the template argument ViewClass.

Yeah the TypedArrayType can be found with ViewClass::TypedArrayStorageType

[Caio Lima]

Comment on attachment 282752 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:216
>> +    if (!valueToFind.isNumber() && !valueToFind.isDouble())
> 
> Isn't "isNumber()" a superset of "isDouble()"?
> 
> valueToFind.isNumber() false implies valueToFind.isDouble() false.

You are right. I was considering isDouble because of NaN case, bit Ironically, NaN isNumber is true.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222
>> +    typename ViewClass::ElementType target = ViewClass::toAdaptorNativeFromValue(exec, valueToFind);
> 
> I don't think that works for includes.
> 
> For example, Uint8ClampedAdaptor will clamp positive values to 255, that's not what we need here.
> IntegralTypedArrayAdaptor does ToInt32 on the input, that's not what we need either.
> 
> Your test should have extensive coverage of those crazy cases.
> 
> ----
> 
> What you probably need is a new Adaptor function that returns a value in the right type or an error.
> 
> For example, if you pass a double to an integer type array, it returns the integer if it fits in the array type or fail.
> E.g.:
>     -The double 256.0 works for int16, fails for int8
>     -The double 254.1 fails for all integers.

Nice point. I am working in that now.

[Caio Lima]

Hey Benjamin and Keith, Thank you for you review. I learned a lot from your comments! I am going to update my patch now following your directions, since all of them makes sense to me.

[Caio Lima]

Created attachment 283021 [details]
Patch

[Build Bot]

Comment on attachment 283021 [details]
Patch

Attachment 283021 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1642077

New failing tests:
js/typed-array-includes.html

[Build Bot]

Created attachment 283025 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283021 [details]
Patch

Attachment 283021 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1642082

New failing tests:
js/typed-array-includes.html

[Build Bot]

Created attachment 283026 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283021 [details]
Patch

Attachment 283021 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1642092

New failing tests:
js/typed-array-includes.html

[Build Bot]

Created attachment 283029 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 283021 [details]
Patch

Attachment 283021 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1642085

New failing tests:
js/typed-array-includes.html

[Build Bot]

Created attachment 283030 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Caio Lima]

Created attachment 283112 [details]
Patch

[Benjamin Poulain]

You seem to have trouble getting test expectations.

What's your workflow to get them?

[Caio Lima]

(In reply to comment #33)
> You seem to have trouble getting test expectations.
> 
> What's your workflow to get them?

Yes, it was a painful process. I built all the webkit and un the test in Safari to get the output. After that I changed the file with the current output but I missed the newline. I uploaded the Patch to check if it was ok, because When I tried to run "run-webkit-test" in the morning, the webkit started compile from scratch agin...Now it is fixed (at least localhost) and I am going to check if the win build is not going to fail.

BTW, I faced a strange behavior...Even with new Float(32|64)Array([NaN]), when searchValue=NaN and I perform a loop through elements, array[i] = NaN and target = NaN but array[i] == target returns false...Can you explain it?

[Caio Lima]

Created attachment 283116 [details]
Patch

[Benjamin Poulain]

(In reply to comment #34)
> (In reply to comment #33)
> > You seem to have trouble getting test expectations.
> > 
> > What's your workflow to get them?
> 
> Yes, it was a painful process. I built all the webkit and un the test in
> Safari to get the output. After that I changed the file with the current
> output but I missed the newline. I uploaded the Patch to check if it was ok,
> because When I tried to run "run-webkit-test" in the morning, the webkit
> started compile from scratch agin...Now it is fixed (at least localhost) and
> I am going to check if the win build is not going to fail.

Ok.

Here are two ways you can update the expectations:

If you build the full WebKit (sometimes necessary, not always), you can run the test you want and use --reset-results.
For example, in your case you can do:
    ./Tools/Scripts/run-webkit-test js/typed-array-includes.html --reset-results
This will run the test and replace the -expected.txt file of that test.
(Note that the command takes the path to the test without "LayoutTest" in front, which is a bit weird)


If you do not need to run any of the WebKit test, you can sometime get away with just building JavaScript.
What you do then is run the script with standalone-pre.js and standalone-post.js. For example for your new test I would try:
    DYLD_FRAMEWORK_PATH=./WebKitBuild/Release/ ./WebKitBuild/Release/jsc LayoutTest/resources/standalone-pre.js LayoutTests/js/script-tests/typed-array-includes.js LayoutTest/resources/standalone-post.js  > LayoutTests/js/typed-array-includes-expected.txt
 
A bit ugly, but it gets the job done.

> BTW, I faced a strange behavior...Even with new Float(32|64)Array([NaN]),
> when searchValue=NaN and I perform a loop through elements, array[i] = NaN
> and target = NaN but array[i] == target returns false...Can you explain it?

I am not certain to understand the question but if you are wondering about NaN equality...:

If you do NaN === NaN, that's false. A NaN is not equal to itself (or any other number).
Even in C, if you have
    double value = NaN;
    (value == value) -> This is false.

Since NaN is not a number, comparing it to anything is false.
In practice, CPUs have two kinds of comparisons: ordered and unordered.
-Ordered comparison is true if neither operands are NaN.
-An unordered comparison is true if either operands are NaN.
If you look at the "DoubleCondition" enum in any of the MacroAssembler, you'll get an idea of what comparison operator are available.

---

If I misunderstood your question: can you give an complete example?

[Caio Lima]

Thank you for these information! They are extremely valuable to my workflow.

It is the reason that I didn't remove the if-else clause checking the NaN case.

Now everything is green, finally. After this patch being r+ and accepted, I am going to resuse the work done here to fix TypedArray.indexOf implementation.

[Benjamin Poulain]

Comment on attachment 283116 [details]
Patch

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

Third review round.
This is getting in really good shape. But I think I found a couple of bugs:

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:209
> +    if (!valueToFind.isNumber())
> +        return JSValue::encode(jsBoolean(false));

I believe having this here is a bug. It is not spec compliant.

If you check https://tc39.github.io/ecma262/2016/#sec-array.prototype.includes, step 4. is calling ToInteger() on the index.
Here, if the argument zero is not a number, we exit *before* calling ToInteger.

Why is that important?
The second argument can be an object with "valueOf". This optimization will skip the call to valueOf.

You should add a test verifying that passing a string as first argument and an object as second argument calls valueOf() exactly once on the object.

> Source/JavaScriptCore/runtime/ToNativeFromValue.h:58
> +    return Adaptor::toNativeFromDouble(value.toNumber(exec), result);

I think this is a bug too.

You are calling toNumber() on the value.
If valueToFind is an Object, that will call valueOf() on it. If it's null or undefined, it will be converted to number too.

You just need to return false here.

You need a test covering this case.

> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:104
> +        Type ans = static_cast<Type>(value);

Rename ans->integer?

> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:163
> +    static Type toNativeFromUint32(uint32_t value, Type& result)

Is this needed? Where is it called?

> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:177
> +        if (value < minValue || value > maxValue)
> +            return false;

This is not really enough because of double->float conversion.

A double can be bigger than minValue and less than maxValue yet not convert cleanly to a float. This happens if the double has a higher precision than the float.

What we need is (static_cast<double>(static_cast<Type>(value)) == value).
This ensure that the conversion to float and back to double does not lose precision.
This is valid because you have a NaN check above so we don't have to deal with non-finite numbers.

It's probably possible to write a test covering this.

> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:278
> +    static bool toNativeFromUint32(uint32_t value, Type& result)

Is this needed?

> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:290
> +        int32_t ans = static_cast<int32_t>(value);

Rename ans->integer.
I believe you could even type it uint8_t and skip the "if (ans > maxValue || ans < minValue)" below.

[Caio Lima]

Comment on attachment 283116 [details]
Patch

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

Good points in comment. Thank you for the contributions, mainly because I didn't pay attention to float -> double case!

>> Source/JavaScriptCore/runtime/ToNativeFromValue.h:58
>> +    return Adaptor::toNativeFromDouble(value.toNumber(exec), result);
> 
> I think this is a bug too.
> 
> You are calling toNumber() on the value.
> If valueToFind is an Object, that will call valueOf() on it. If it's null or undefined, it will be converted to number too.
> 
> You just need to return false here.
> 
> You need a test covering this case.

Actually, we have test covering this case. Check LayoutTests/js/script-tests/typed-array-includes.js:17-23.

When this function is called from "genericTypedArrayViewProtoFuncIncludes", we are sure that value.isNumber() is true. However, maintaining this last case I think it keeps the function more reusable.

[Caio Lima]

Created attachment 283270 [details]
Patch

[Build Bot]

Comment on attachment 283270 [details]
Patch

Attachment 283270 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1654942

New failing tests:
js/typed-array-includes.html

[Build Bot]

Created attachment 283271 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283270 [details]
Patch

Attachment 283270 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1654945

New failing tests:
js/typed-array-includes.html

[Build Bot]

Created attachment 283272 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283270 [details]
Patch

Attachment 283270 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1654949

New failing tests:
js/typed-array-includes.html

[Build Bot]

Created attachment 283273 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283270 [details]
Patch

Attachment 283270 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1654948

New failing tests:
js/typed-array-includes.html

[Build Bot]

Created attachment 283274 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Caio Lima]

Created attachment 283277 [details]
Patch

[Benjamin Poulain]

Comment on attachment 283277 [details]
Patch

Fantastic work. Great job on the test coverage.
Let's land!

[Caio Lima]

(In reply to comment #50)
> Comment on attachment 283277 [details]
> Patch
> 
> Fantastic work. Great job on the test coverage.
> Let's land!

Thank you very much! I really liked your review because I learned a lot of new things and also we could catch a lot of "hidden" cases. It was a good example that 2 heads think better than one =).

Tomorrow morning I am going to start work in the %TypedArray%.prototype.indexOf bug and this patch is going to be helpful there.

[WebKit Commit Bot]

Comment on attachment 283277 [details]
Patch

Clearing flags on attachment: 283277

Committed r203037: <http://trac.webkit.org/changeset/203037>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/3404/
https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/7277

It seems that this patch causes JSC test failures. Could you take a look?

[Caio Lima]

(In reply to comment #54)
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/3404/
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/7277
> 
> It seems that this patch causes JSC test failures. Could you take a look?

Hey Yusuke, is it possible get the output of this LayoutTests run?

[Yusuke Suzuki]

(In reply to comment #55)
> (In reply to comment #54)
> > https://build.webkit.org/builders/
> > Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/3404/
> > https://build.webkit.org/builders/
> > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/7277
> > 
> > It seems that this patch causes JSC test failures. Could you take a look?
> 
> Hey Yusuke, is it possible get the output of this LayoutTests run?

The results of the layout-tests can be seen in https://build.webkit.org/results/ :)

[Caio Lima]

(In reply to comment #56)
> (In reply to comment #55)
> > (In reply to comment #54)
> > > https://build.webkit.org/builders/
> > > Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/3404/
> > > https://build.webkit.org/builders/
> > > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/7277
> > > 
> > > It seems that this patch causes JSC test failures. Could you take a look?
> > 
> > Hey Yusuke, is it possible get the output of this LayoutTests run?
> 
> The results of the layout-tests can be seen in
> https://build.webkit.org/results/ :)

Well, the problem is weird. When I run "run-javascriptcore-tests", the output is different from "run-webkit-tests". Here is the diff:

--- /Users/caiolima/open_projects/WebKit/WebKitBuild/Debug/layout-test-results/js/typed-array-includes-expected.txt
+++ /Users/caiolima/open_projects/WebKit/WebKitBuild/Debug/layout-test-results/js/typed-array-includes-actual.txt
@@ -1,3 +1,8 @@
+JSRegress/typed-array-includes
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
 PASS tArray.includes(2) is true
 PASS !tArray.includes(4) is true
 PASS !tArray.includes(3, 3) is true
@@ -251,6 +256,7 @@
 PASS funcCallCount.callCount === 4 is true
 PASS funcCallCount.callCount === 5 is true
 PASS funcCallCount.callCount === 6 is true
+PASS no exception thrown
 PASS successfullyParsed is true
 
 TEST COMPLETE

I can move the test from LayoutTests to Source/JavaScriptCore/tests and avoid it breaks the "LayoutTests". However, I would like to know why this difference happens. Do you have any tips?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 159614

[Caio Lima]

Created attachment 283383 [details]
Patch

[Benjamin Poulain]

Comment on attachment 283383 [details]
Patch

I'll wait for the bots before cq+.

[Caio Lima]

(In reply to comment #60)
> Comment on attachment 283383 [details]
> Patch
> 
> I'll wait for the bots before cq+.

It is green now.

[Benjamin Poulain]

(In reply to comment #60)
> Comment on attachment 283383 [details]
> Patch
> 
> I'll wait for the bots before cq+.

Sorry, I was on the road.

[WebKit Commit Bot]

Comment on attachment 283383 [details]
Patch

Clearing flags on attachment: 283383

Committed r203107: <http://trac.webkit.org/changeset/203107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Shvayka]

*** Bug 156982 has been marked as a duplicate of this bug. ***