[webkit-reviews] review denied: [Bug 125391] new Int32Array(new ArrayBuffer(100), 1, 1) shouldn't throw an error that says "RangeError: Byte offset and length out of range of buffer" : [Attachment 235071] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Diego Pino <dpino at igalia.com>'s
request for review:
Bug 125391: new Int32Array(new ArrayBuffer(100), 1, 1) shouldn't throw an error
that says "RangeError: Byte offset and length out of range of buffer"
https://bugs.webkit.org/show_bug.cgi?id=125391

Attachment 235071: Patch
https://bugs.webkit.org/attachment.cgi?id=235071&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235071&action=review


Looks good.

But there is no test coverage for alignment problems. The tests only test the
string change for out of range problems, not the new code to check for
alignment problems.

Also, when there is both an alignment problem and an out of range problem,
which error takes precedence? The tests should cover that and the code should
be structured to work as expected. I don’t have a preference, but it would be
good to have this be well defined.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:82
> +    template <typename T>
> +    static bool verifyByteOffsetAlignment(unsigned byteOffset)

I think it’s overkill that we use a template here just so we can call
sizeof(T). Doesn’t seem right. Could just be an inline function where we pass
the size.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:84
> +	   return !(byteOffset & (sizeof(T) - 1));

This assumes that sizeof(T) is a power of 2 without checking that it is. The
old code used modulus which does not make that assumption. Can we change this
back or is doing it this way an important optimization?

The old code was optimized/specialized for the size 1, but here that
optimization was removed. Was that optimization not important? If it is
important, it’s easy to re-create.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:89
>      template <typename T>

I think it’s overkill that we use a template here just so we can call
sizeof(T). Doesn’t seem right. Could just be an inline function where we pass
the size.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:91
> +    static bool verifySubRangeLength(PassRefPtr<ArrayBuffer> buffer,
> +	   unsigned byteOffset, unsigned numElements)

This should all be on one line.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:-92
> -	   if (sizeof(T) > 1 && byteOffset % sizeof(T))
> -	       return false;

Why remove the optimization for size 1? Was that optimization not important? If
it is important, we could probably write it a lot better.

> Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h:67
>	   return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSDataView.cpp:52
>	   throwVMError(
> -	       exec, createRangeError(exec, "Byte offset and length out of
range of buffer"));
> +	       exec, createRangeError(exec, "Length out of range of buffer"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSDataView.cpp:53
> +	   return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSDataView.cpp:57
> +	   exec->vm().throwException(
> +	       exec, createRangeError(exec, "Byte offset is not aligned"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSDataView.cpp:58
>	   return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:88
>	   exec->vm().throwException(
> -	       exec, createRangeError(exec, "Byte offset and length out of
range of buffer"));
> +	       exec, createRangeError(exec, "Length out of range of buffer"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:89
> +	   return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:93
> +	   exec->vm().throwException(
> +	       exec, createRangeError(exec, "Byte offset is not aligned"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:94
>	   return 0;

Should be nullptr.