Bug 148035 - [ES6] Add TypedArray.prototype functionality.
Summary: [ES6] Add TypedArray.prototype functionality.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
: 138218 (view as bug list)
Depends on: 148560 149694 149697
Blocks: 149687
  Show dependency treegraph
 
Reported: 2015-08-14 12:56 PDT by Keith Miller
Modified: 2020-06-07 23:51 PDT (History)
10 users (show)

See Also:


Attachments
Patch (155.68 KB, patch)
2015-08-14 13:05 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (155.60 KB, patch)
2015-08-14 15:21 PDT, Keith Miller
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (581.19 KB, application/zip)
2015-08-14 16:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-mavericks (817.40 KB, application/zip)
2015-08-14 16:19 PDT, Build Bot
no flags Details
Patch (159.25 KB, patch)
2015-08-14 18:15 PDT, Keith Miller
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (603.56 KB, application/zip)
2015-08-14 18:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (579.68 KB, application/zip)
2015-08-14 19:03 PDT, Build Bot
no flags Details
Patch (159.36 KB, patch)
2015-08-17 11:28 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (159.39 KB, patch)
2015-08-17 15:45 PDT, Keith Miller
ggaren: review-
ggaren: commit-queue-
Details | Formatted Diff | Diff
Patch (162.25 KB, patch)
2015-08-19 11:33 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (159.39 KB, patch)
2015-08-19 16:24 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (162.27 KB, patch)
2015-08-19 16:27 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (162.97 KB, patch)
2015-08-21 16:56 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (162.97 KB, patch)
2015-08-24 14:36 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (163.02 KB, patch)
2015-08-26 13:57 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (163.12 KB, patch)
2015-08-26 17:04 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (163.12 KB, patch)
2015-08-31 13:58 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (134.66 KB, patch)
2015-09-03 15:40 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (134.66 KB, patch)
2015-09-04 13:08 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (383.47 KB, patch)
2015-09-04 13:59 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (135.03 KB, patch)
2015-09-04 14:31 PDT, Keith Miller
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2015-08-14 12:56:16 PDT
This patch should add most of the functionality for the prototype properties of TypedArray objects in ES6. There are a few exceptions to this, which will be added in upcoming patches:

1) First we do not use the species constructor for some of the TypedArray prototype functions (namely: map, filter, slice, and subarray). That will need to be added when species constructors are finished.

2) TypedArrays still have a length, byteOffset, byteLength and buffer are still attached to the TypedArray instance (in the spec they are on the TypedArray.prototype instance object) since the JIT currently assumes those properties are fixed.

3) The TypedArray.constructor property is not added yet as it should point to the TypedArray instance object, which is added in a future patch.
Comment 1 Keith Miller 2015-08-14 13:05:17 PDT
Created attachment 259030 [details]
Patch
Comment 2 Sam Weinig 2015-08-14 14:14:14 PDT
Comment on attachment 259030 [details]
Patch

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

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:30
> +function every(callback /*, thisArg */) {
> +    "use strict";
> +    var length = @typedArrayLength(this);

I recently unified our style for the builtins to be:

function functionName(arguments)
{
    "use strict";

    rest of function goes here
}

It would be great if we could keep it consistent.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:98
> +    for (var i = 0; i < length; i++)
> +        if (callback.@call(thisArg, this[i], i, this))
> +            return true;

This for loop should have braces.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:343
> +    // This is matching FireFox behavior. In particular, it rejects all attempts to

This seems unrelated and I don't think it is correct.  Firefox is spelled as one word.
Comment 3 Keith Miller 2015-08-14 14:59:06 PDT
Comment on attachment 259030 [details]
Patch

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

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:30
>> +    var length = @typedArrayLength(this);
> 
> I recently unified our style for the builtins to be:
> 
> function functionName(arguments)
> {
>     "use strict";
> 
>     rest of function goes here
> }
> 
> It would be great if we could keep it consistent.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:98
>> +            return true;
> 
> This for loop should have braces.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:343
>> +    // This is matching FireFox behavior. In particular, it rejects all attempts to
> 
> This seems unrelated and I don't think it is correct.  Firefox is spelled as one word.

Whoops, I was trying to figure out why that line was appearing in my diff but I couldn't figure it out... That would explain it.
Comment 4 Keith Miller 2015-08-14 15:21:57 PDT
Created attachment 259051 [details]
Patch

Should build now. Also, addressed comments.
Comment 5 Build Bot 2015-08-14 16:09:49 PDT
Comment on attachment 259051 [details]
Patch

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

New failing tests:
fast/canvas/webgl/type-conversion-test.html
Comment 6 Build Bot 2015-08-14 16:09:53 PDT
Created attachment 259053 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 7 Build Bot 2015-08-14 16:19:31 PDT
Comment on attachment 259051 [details]
Patch

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

New failing tests:
fast/canvas/webgl/type-conversion-test.html
fast/dom/Window/window-postmessage-clone.html
Comment 8 Build Bot 2015-08-14 16:19:33 PDT
Created attachment 259055 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 9 Keith Miller 2015-08-14 18:15:02 PDT
Created attachment 259061 [details]
Patch
Comment 10 Build Bot 2015-08-14 18:51:51 PDT
Comment on attachment 259061 [details]
Patch

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

New failing tests:
fast/canvas/webgl/type-conversion-test.html
Comment 11 Build Bot 2015-08-14 18:51:54 PDT
Created attachment 259065 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 12 Build Bot 2015-08-14 19:03:19 PDT
Comment on attachment 259061 [details]
Patch

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

New failing tests:
fast/canvas/webgl/type-conversion-test.html
Comment 13 Build Bot 2015-08-14 19:03:21 PDT
Created attachment 259069 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 14 Keith Miller 2015-08-17 11:28:02 PDT
Created attachment 259166 [details]
Patch
Comment 15 Keith Miller 2015-08-17 15:45:38 PDT
Created attachment 259195 [details]
Patch
Comment 16 Geoffrey Garen 2015-08-18 15:46:52 PDT
Comment on attachment 259195 [details]
Patch

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

Looks good but needs some improvement.

> LayoutTests/ChangeLog:8
> +        Added tests for the TypedArray.prototype functions

functions => functions.

> LayoutTests/ChangeLog:9
> +        all the tests use the typedarray-test-helper-function.js

all => All

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:26
> +// Note that the intrisic @typedArrayLength checks the that the argument passed is a typed array.

Does it throw if not? You should say so.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:119
> +    function merge(dst, src, srcIndex, srcEnd, width, comparator)

Because typed arrays do not require stable sort (since their entries are all primitives), it would be nice to use a faster sorting algorithm, like quick sort. You should switch to quick sort in a follow-up patch.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:168
> +
> +        mergeSort(this, length, comparator);

Stray newline.

No braces for single-line block.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:191
> +    else {

No braces for single-line block.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:217
> +    else {

No braces for single-line block.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:238
> +    // FIXME: This should be a species constructor

constructor => constructor.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:266
> +    // FIXME: This should be a species constructor

constructor => constructor.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:269
> +    for (var i = 0; i < kept.length; i++) {

No braces for single-line block.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:89
> +

Stray newline.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:102
> +    TypeConverter* unionArray = bitwise_cast<TypeConverter*>(floatArray);

This.... probably doesn't do what you think it does.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:110
> +    // When we sort floating point numbers as ints comparing two negative numbers will
> +    // give the wrong ordering so we need to reverse these numbers.

Can you pass a comparator to std::sort that will do this naturally?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:225
> +

Stray newline.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:240
> +        default: {

No braces for single line statement.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:242
> +            }

It's good to break, even at the end.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:207
> +template<typename ViewClass>
> +EncodedJSValue JSC_HOST_CALL genericTypedArrayViewProtoFuncToLocaleString(ExecState* exec)

No JavaScript action on this one? It would help with the function call.
Comment 17 Geoffrey Garen 2015-08-18 16:13:58 PDT
Comment on attachment 259195 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:102
>> +    TypeConverter* unionArray = bitwise_cast<TypeConverter*>(floatArray);
> 
> This.... probably doesn't do what you think it does.

To clarify here, our strategy is to read the array as if it were a union, to avoid strict aliasing violation. That means the initial cast of the pointer to first member can be reinterpret_cast, and all subsequent access should be through the union type, which means we should not bitwise cast below, and we should sort an array of union and not an array of int.
Comment 18 Yusuke Suzuki 2015-08-18 17:55:20 PDT
Comment on attachment 259195 [details]
Patch

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

Great, significant work :D

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:87
> +void sortFloat(FloatType* floatArray, unsigned length)

I suggest you to write why this function works.
Or I suggest you to write this as a simple std::sort at first (and later, optimize it with the benchmark :D)

(the following one is my rough understanding),

IEEE 754 ensures that the mantissa and exponential part of the format (63bit) is ordered except for NaN.
Because IEEE 754 does not use complement representation for the negative values, the negative float numbers recognized as the integral numbers are reverse ordered.
We summarize the result of the comparison between the floating numbers recognized as the integral ones.

- <=> - = reversed (-30 > -20 = true)
+ <=> + = ordered (30 > 20 = true)
- <=> + = ordered (-30 > 20 = false)
+ <=> - = ordered (30 > -20 = true)

As a result, when sorting the array,
1. unsigned numbers are correctly ordered.
2. The order of unsigned and signed numbers are correctly kept.
3. But the order between signed numbers are reversed.



For NaN, we normalize the NaN to the specific representation; sign bit is 0, all exponential part is 1 and the MSB of the mantissa is 1.
So NaN is recognized as the largest integral numbers.

I suggest you to test with the following edge numbers,

1. positive infinity, negative infinity
2. -0.0, +0.0
3. NaN
4. denormalized numbers

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:104
> +        unionArray[i].floatType = purifyNaN(unionArray[i].floatType);

I think, here, we can just write

for (unsigned i = 0; i < length; ++i)
   floatArray[i] = purifyNaN(floatArray[i]);

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:106
> +    IntegralType* intArray = bitwise_cast<IntegralType*>(unionArray);

Can we just write `bitwise_cast<IntegralType*>(floatArray)`?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:112
> +    while (lastNegativeIndex < length && std::signbit(unionArray[++lastNegativeIndex].floatType)) { }

Let's replace `{ }` to `;`.
I think we can check it by using intArray (intArray[i] < 0) instead of using `std::signbit`.

And this code has the issue of overrun. When `length == 1`, this will access `unionArray[1]`.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:108
> +    long final = argumentClampedIndexFromStartOrEnd(exec, 2, length, length);

Why are they `long`?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:182
> +    StringView separator;

StringView is just the view and it does not have any ownership of the held string value.
So when using it, we need to take care of the lifetime of the owner string.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:185
> +        separator = { &comma, 1 };

Is this &comma addressing safe? This comma address is the address of the stack.
When this brace is ended, the lifetime of the variable `comma` is finished.
I think we need to change it to the following.

const LChar* comma = ",";
separator = { comma, 1 };

String Literal is allocated in the static area and its lifetime is the same to the program.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:190
> +        separator = separatorString->view(exec);

If we would like to keep separator valid, we need to maintain separatorString alive.
I suggest you to move `JSString* separatorString` to the function top-level block to make it explicit that separatorString is alive during this function.

> Source/JavaScriptCore/runtime/JSObject.h:1492
> +    } while (false)

Nice!

> Source/JavaScriptCore/runtime/JSTypedArrayPrototypes.cpp:35
> +    CREATE_METHOD_TABLE(JSTypedArrayViewPrototype)};

We can just write `&Base::s_info` here.

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:69
> +        return throwVMError(exec, createTypeError(exec, "Receiver should be a typed array view"));

Could you add "Receiver should be a typed array view but was not an object" test cases for each function?
Comment 19 Keith Miller 2015-08-19 11:29:56 PDT
Comment on attachment 259195 [details]
Patch

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

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:26
>> +// Note that the intrisic @typedArrayLength checks the that the argument passed is a typed array.
> 
> Does it throw if not? You should say so.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:168
>> +        mergeSort(this, length, comparator);
> 
> Stray newline.
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:191
>> +    else {
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:217
>> +    else {
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:238
>> +    // FIXME: This should be a species constructor
> 
> constructor => constructor.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:266
>> +    // FIXME: This should be a species constructor
> 
> constructor => constructor.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:269
>> +    for (var i = 0; i < kept.length; i++) {
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:87
>> +void sortFloat(FloatType* floatArray, unsigned length)
> 
> I suggest you to write why this function works.
> Or I suggest you to write this as a simple std::sort at first (and later, optimize it with the benchmark :D)
> 
> (the following one is my rough understanding),
> 
> IEEE 754 ensures that the mantissa and exponential part of the format (63bit) is ordered except for NaN.
> Because IEEE 754 does not use complement representation for the negative values, the negative float numbers recognized as the integral numbers are reverse ordered.
> We summarize the result of the comparison between the floating numbers recognized as the integral ones.
> 
> - <=> - = reversed (-30 > -20 = true)
> + <=> + = ordered (30 > 20 = true)
> - <=> + = ordered (-30 > 20 = false)
> + <=> - = ordered (30 > -20 = true)
> 
> As a result, when sorting the array,
> 1. unsigned numbers are correctly ordered.
> 2. The order of unsigned and signed numbers are correctly kept.
> 3. But the order between signed numbers are reversed.
> 
> 
> 
> For NaN, we normalize the NaN to the specific representation; sign bit is 0, all exponential part is 1 and the MSB of the mantissa is 1.
> So NaN is recognized as the largest integral numbers.
> 
> I suggest you to test with the following edge numbers,
> 
> 1. positive infinity, negative infinity
> 2. -0.0, +0.0
> 3. NaN
> 4. denormalized numbers

Added Comment

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:89
>> +
> 
> Stray newline.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:225
>> +
> 
> Stray newline.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:240
>> +        default: {
> 
> No braces for single line statement.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:242
>> +            }
> 
> It's good to break, even at the end.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:108
>> +    long final = argumentClampedIndexFromStartOrEnd(exec, 2, length, length);
> 
> Why are they `long`?

I made them long so no overflow would occur in computations for count.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:185
>> +        separator = { &comma, 1 };
> 
> Is this &comma addressing safe? This comma address is the address of the stack.
> When this brace is ended, the lifetime of the variable `comma` is finished.
> I think we need to change it to the following.
> 
> const LChar* comma = ",";
> separator = { comma, 1 };
> 
> String Literal is allocated in the static area and its lifetime is the same to the program.

I believe you are correct. I just copied this code from ArrayPrototype.cpp and didn't give it too much thought. We should probably fix this there as well.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:190
>> +        separator = separatorString->view(exec);
> 
> If we would like to keep separator valid, we need to maintain separatorString alive.
> I suggest you to move `JSString* separatorString` to the function top-level block to make it explicit that separatorString is alive during this function.

I don't think there is an issue here. I believe JSString* lives in the js heap so it needs to be marked before it can be GCed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:207
>> +EncodedJSValue JSC_HOST_CALL genericTypedArrayViewProtoFuncToLocaleString(ExecState* exec)
> 
> No JavaScript action on this one? It would help with the function call.

Yeah, you are probably right. I moved this to a builtin.

>> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:69
>> +        return throwVMError(exec, createTypeError(exec, "Receiver should be a typed array view"));
> 
> Could you add "Receiver should be a typed array view but was not an object" test cases for each function?

Fixed.
Comment 20 Keith Miller 2015-08-19 11:33:28 PDT
Created attachment 259381 [details]
Patch
Comment 21 Keith Miller 2015-08-19 16:24:11 PDT
Created attachment 259425 [details]
Patch

Per offline discussion, I realized that it is necessary to maintain the JSString* throughout the lifetime of Join.
Comment 22 Keith Miller 2015-08-19 16:27:13 PDT
Created attachment 259427 [details]
Patch

Uploaded the wrong file...
Comment 23 Geoffrey Garen 2015-08-21 15:46:20 PDT
Comment on attachment 259427 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:37
> +#define CALL_GENERIC_TYPEDARRAY_PROTOTYPE_FUNCTION(functionName) do {                           \

You should just use an inline function here. It's easier to debug and not any slower.

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:65
> +static EncodedJSValue JSC_HOST_CALL typedArrayViewPrivateFuncLength(ExecState* exec)

Why are all of these functions in an Inlines.h file? It seems that we never inline them.

I think all of this stuff belongs in a .cpp file somewhere.
Comment 24 Keith Miller 2015-08-21 16:51:13 PDT
Comment on attachment 259427 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:37
>> +#define CALL_GENERIC_TYPEDARRAY_PROTOTYPE_FUNCTION(functionName) do {                           \
> 
> You should just use an inline function here. It's easier to debug and not any slower.

Unfortunately, I don't think that's possible as the function is a templated thing and C++ can't take templated functions as parameters. At least as far as I know.

>> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:65
>> +static EncodedJSValue JSC_HOST_CALL typedArrayViewPrivateFuncLength(ExecState* exec)
> 
> Why are all of these functions in an Inlines.h file? It seems that we never inline them.
> 
> I think all of this stuff belongs in a .cpp file somewhere.

That was an oversight on my part. At one point I had some template functions in here but when I removed them I forgot to move everything to a .cpp file. It's fixed now.
Comment 25 Keith Miller 2015-08-21 16:56:19 PDT
Created attachment 259684 [details]
Patch
Comment 26 Keith Miller 2015-08-24 14:36:23 PDT
Created attachment 259773 [details]
Patch

Trunk looked broken trying again.
Comment 27 Keith Miller 2015-08-26 13:57:10 PDT
Created attachment 259979 [details]
Patch
Comment 28 Keith Miller 2015-08-26 17:04:06 PDT
Created attachment 259996 [details]
Patch
Comment 29 Geoffrey Garen 2015-08-27 14:49:53 PDT
Comment on attachment 259996 [details]
Patch

r=me
Comment 30 WebKit Commit Bot 2015-08-27 15:57:25 PDT
Comment on attachment 259996 [details]
Patch

Clearing flags on attachment: 259996

Committed r189064: <http://trac.webkit.org/changeset/189064>
Comment 31 WebKit Commit Bot 2015-08-27 15:57:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Jordan Harband 2015-08-27 16:19:36 PDT
This is awesome! What about Array#includes and TypedArray#includes? These are stage 3 in ES7, and as such are ready for browser implementations.
Comment 33 Brent Fulgham 2015-08-27 16:27:06 PDT
This broke the Windows build.
Comment 34 Brent Fulgham 2015-08-27 16:30:50 PDT
Missing symbols:

     Creating library C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\JavaScriptCore.lib and object C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\JavaScriptCore.exp
JSGlobalObject.obj : error LNK2019: unresolved external symbol "public: static class JSC::JSTypedArrayViewPrototype * __cdecl JSC::JSTypedArrayViewPrototype::create(class JSC::VM &,class JSC::JSGlobalObject *,class JSC::Structure *)" (?create@JSTypedArrayViewPrototype@JSC@@SAPAV12@AAVVM@2@PAVJSGlobalObject@2@PAVStructure@2@@Z) referenced in function "private: void __thiscall JSC::JSGlobalObject::init(class JSC::VM &)" (?init@JSGlobalObject@JSC@@AAEXAAVVM@2@@Z) [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]
JSGlobalObject.obj : error LNK2019: unresolved external symbol "public: static class JSC::Structure * __cdecl JSC::JSTypedArrayViewPrototype::createStructure(class JSC::VM &,class JSC::JSGlobalObject *,class JSC::JSValue)" (?createStructure@JSTypedArrayViewPrototype@JSC@@SAPAVStructure@2@AAVVM@2@PAVJSGlobalObject@2@VJSValue@2@@Z) referenced in function "private: void __thiscall JSC::JSGlobalObject::init(class JSC::VM &)" (?init@JSGlobalObject@JSC@@AAEXAAVVM@2@@Z) [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]
JSGlobalObject.obj : error LNK2019: unresolved external symbol "__int64 __fastcall JSC::typedArrayViewPrivateFuncSort(class JSC::ExecState *)" (?typedArrayViewPrivateFuncSort@JSC@@YI_JPAVExecState@1@@Z) referenced in function "private: void __thiscall JSC::JSGlobalObject::init(class JSC::VM &)" (?init@JSGlobalObject@JSC@@AAEXAAVVM@2@@Z) [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]
JSGlobalObject.obj : error LNK2019: unresolved external symbol "__int64 __fastcall JSC::typedArrayViewPrivateFuncLength(class JSC::ExecState *)" (?typedArrayViewPrivateFuncLength@JSC@@YI_JPAVExecState@1@@Z) referenced in function "private: void __thiscall JSC::JSGlobalObject::init(class JSC::VM &)" (?init@JSGlobalObject@JSC@@AAEXAAVVM@2@@Z) [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin32\JavaScriptCore.dll : fatal error LNK1120: 4 unresolved externals [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]
Comment 35 Filip Pizlo 2015-08-27 17:09:10 PDT
This broke JSC stress testing because of the typedarray-test-helper-function.js file, which the stress test harness thinks is a test.
Comment 36 Csaba Osztrogonác 2015-08-27 21:35:47 PDT
(In reply to comment #35)
> This broke JSC stress testing because of the
> typedarray-test-helper-function.js file, which the stress test harness
> thinks is a test.

Mark already fixed it in https://trac.webkit.org/changeset/189084 .

But the Windows build is still broken ... Otherwise why did 
you land it without running tests and fixing Windows build?
Comment 37 WebKit Commit Bot 2015-08-27 22:14:43 PDT
Re-opened since this is blocked by bug 148560
Comment 38 Mark Lam 2015-08-27 22:22:09 PDT
FYI, rolled out in r189085: <http://trac.webkit.org/changeset/189085>
Comment 39 Keith Miller 2015-08-27 22:25:36 PDT
Sorry, about the issues. I didn't realize the javascript tests were not run on EWS. I'll roll out the patch and fix theses things
Comment 40 Csaba Osztrogonác 2015-08-28 01:58:57 PDT
(In reply to comment #38)
> FYI, rolled out in r189085: <http://trac.webkit.org/changeset/189085>

Brent landed a Windows buildfix in https://trac.webkit.org/changeset/189077
(necessary, but not enough) and I reverted it in https://trac.webkit.org/changeset/189090 to fix the Windows build after the rollout.
Comment 41 Csaba Osztrogonác 2015-08-28 02:14:20 PDT
Brent landed one more buildfix - https://trac.webkit.org/changeset/189072
I reverted the part of r189072 which is responsible for fixing this bug:
https://trac.webkit.org/changeset/189091
Comment 42 Keith Miller 2015-08-31 13:58:54 PDT
Created attachment 260315 [details]
Patch

Moved tests out of LayoutTests and into JavascriptCore/tests/stress which should fix the testing problems. Hopefully, the Windows build is fixed but I'll check the EWS results.
Comment 43 Keith Miller 2015-08-31 14:20:29 PDT
Windows is still not building. I'll have to find a Windows machine to debug it.
Comment 44 Yusuke Suzuki 2015-08-31 20:31:43 PDT
Comment on attachment 260315 [details]
Patch

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

You need to add introduced *.cpp and *.h to JavaScriptCore.vcxproj/JavaScriptCore.vcxproj and JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters to build sources in Windows.

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototype.cpp:69
> +EncodedJSValue typedArrayViewPrivateFuncLength(ExecState* exec)

Let's add JSC_HOST_CALL annotation. It enforces __fastcall calling convention in Windows environment.

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototype.cpp:78
> +EncodedJSValue typedArrayViewPrivateFuncSort(ExecState* exec)

Let's add JSC_HOST_CALL annotation. It enforces __fastcall calling convention in Windows environment.
Comment 45 Keith Miller 2015-09-03 15:40:10 PDT
Created attachment 260533 [details]
Patch

Fixed the Windows build for real this time... hopefully.
Comment 46 Keith Miller 2015-09-03 16:44:38 PDT
Welp, things are still broken... On the plus side it doesn't look like it's my changes. I'll try running again later
Comment 47 Keith Miller 2015-09-04 13:08:53 PDT
Created attachment 260608 [details]
Patch

Running windows build again since the last time the error looked unrelated
Comment 48 Keith Miller 2015-09-04 13:59:41 PDT
Created attachment 260614 [details]
Patch

Other people shouldn't be allowed to add files to Xcode...
Comment 49 WebKit Commit Bot 2015-09-04 14:02:24 PDT
Attachment 260614 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:27:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: LayoutTests/ChangeLog:19:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 50 Keith Miller 2015-09-04 14:31:48 PDT
Created attachment 260621 [details]
Patch

Didn't merge ChangeLogs Correctly.
Comment 51 Geoffrey Garen 2015-09-29 13:42:11 PDT
Comment on attachment 260621 [details]
Patch

r=me
Comment 52 Keith Miller 2015-09-30 14:01:09 PDT
Committed r190367: <http://trac.webkit.org/changeset/190367>
Comment 53 Brent Fulgham 2015-09-30 14:29:27 PDT
This seems to have introduced a build failure on Windows. Are these new symbols?

C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(219): error C2065: 'typedArrayPrototypeEveryCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(220): error C2065: 'typedArrayPrototypeFilterCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(221): error C2065: 'typedArrayPrototypeSortCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(224): error C2065: 'typedArrayPrototypeFindCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(225): error C2065: 'typedArrayPrototypeFindIndexCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(226): error C2065: 'typedArrayPrototypeForEachCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(232): error C2065: 'typedArrayPrototypeMapCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(233): error C2065: 'typedArrayPrototypeReduceCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(234): error C2065: 'typedArrayPrototypeReduceRightCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(238): error C2065: 'typedArrayPrototypeSomeCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(240): error C2065: 'typedArrayPrototypeToLocaleStringCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
Comment 54 Keith Miller 2015-09-30 16:01:03 PDT
I was hoping that the fact that it merged cleanly meant that it would still build. Fixing
Comment 55 Keith Miller 2015-09-30 16:39:20 PDT
Windows build should be fixed with http://trac.webkit.org/changeset/190373
Comment 56 Simon Fraser (smfr) 2015-09-30 21:09:30 PDT
Windows build is still failing. I'm going to roll out so that you can fix Windows offline.

  JSTypedArrayViewPrototype.cpp
C:\cygwin\home\buildbot\slave\win-debug\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(219): error C2065: 'typedArrayPrototypeEveryCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\JavaScriptCore.vcxproj]
Comment 57 WebKit Commit Bot 2015-09-30 21:12:09 PDT
Re-opened since this is blocked by bug 149694
Comment 58 Keith Miller 2015-10-01 13:20:40 PDT
Committed r190429: <http://trac.webkit.org/changeset/190429>
Comment 59 Alexey Shvayka 2020-06-07 23:51:06 PDT
*** Bug 138218 has been marked as a duplicate of this bug. ***