[Webkit-unassigned] [Bug 159385] ECMAScript 2016: %TypedArray%.prototype.includes implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=159385

--- Comment #6 from Caio Lima <ticaiolima at gmail.com> ---
(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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160704/ec74b5a1/attachment.html>