-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Editorial: Cleanup some ArrayBuffer/TypedArray/DataView/Atomics text #3629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
spec.html
Outdated
1. Else, | ||
1. Let _type_ be TypedArrayElementType(_typedArray_). | ||
1. If IsUnclampedIntegerElementType(_type_) is *false* and IsBigIntElementType(_type_) is *false*, throw a *TypeError* exception. | ||
1. Let _type_ be TypedArrayElementType(_typedArray_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't unconditionally throw in the above branch, so why is it safe to take this out of the else
branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether or not to throw a TypeError from ValidateIntegerTypedArray after passing ValidateTypedArray is dependent upon the shape of typedArray—its element type must be matched by either IsUnclampedIntegerElementType or IsBigIntElementType (and when waitable is true, typedArray is further restricted to either an Int32Array [element type INT32, matched by the former] or BigInt64Array [element type BIGINT64, matched by the latter]). These are precisely the cases covered by IsNoTearConfiguration with order SEQ-CST, and for precisely the same reason.
So the behavior doesn't change, it just gets consolidated.
- before: If waitable is true, perform narrow checks against TypedArray constructor name, else duplicate IsNoTearConfiguration against element type.
- after: If waitable is true, perform narrow checks against TypedArray constructor name. Then, regardless of waitable (but guaranteed to pass when waitable is true), require IsNoTearConfiguration against element type.
spec.html
Outdated
|
||
<emu-clause id="sec-toelementcategory" type="abstract operation"> | ||
<h1> | ||
ToElementCategory ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this name. Maybe ToTypedArrayElement
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I started with a similar concept, but the problem is that a returned value might still be subject to overflow/clamping/rounding/etc. before it can actually be used as a TypedArray element—this function just coerces to the right of numeric "category" (Number vs. BigInt), a concept which can also be made explicit in e.g. The TypedArray Constructors as it is in TypedArray instance internal slot [[ContentType]] NUMBER vs. BIGINT.
I'd actually rather use "type" (i.e., To[TypedArray]ElementType), corresponding with ECMAScript Data Types and Values, but can't do so without first renaming the existing "TypedArray element type" concept to something like "[TypedArray] binary element type".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ToNumericForTypedArray
maybe? We already have ToNumeric
for "to either Number or BigInt".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me. Done.
spec.html
Outdated
1. Let _arrayTypeName_ be _typedArray_.[[TypedArrayName]]. | ||
1. If _arrayTypeName_ is *"BigInt64Array"*, let _v_ be ? ToBigInt64(_value_). | ||
1. Else, let _v_ be ? ToInt32(_value_). | ||
1. Let _conversionOperation_ be the abstract operation named in the Conversion Operation column in <emu-xref href="#table-the-typedarray-constructors"></emu-xref> for Constructor Name _typedArray_.[[TypedArrayName]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this an improvement? It can only result in ToBigInt64
or ToInt32
, so I'd rather have that logic immediately available instead of having to look it up in a table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few reasons:
- This algorithm has lots of steps, and using helper operations for argument processing supports more focus on its actual purpose.
- It's not locally clear at this site that the only two possible values of typedArray.[[TypedArrayName]] are "BigInt64Array" and "Int32Array", and adding such an assert would further exacerbate the preceding issue.
- It's not even relevant that those are the only two possibilities, because what's really warranted here is just an appropriate coercion of v for typedArray that would still look the same if ValidateIntegerTypedArray were expanded to support more TypedArray constructors.
This approach could even be taken further with an operation encapsulating the indirection, which presumably would be named ToTypedArrayElement (and also used by NumericToRawBytes):
- 1. Let _conversionOperation_ be the abstract operation named in the Conversion Operation column in <emu-xref href="#table-the-typedarray-constructors"></emu-xref> for Constructor Name _typedArray_.[[TypedArrayName]].
- 1. Let _v_ be ? _conversionOperation_(_value_).
+ 1. Let _v_ be ? ToTypedArrayElement(_value_, TypedArrayElementType(_typedArray_)).
1. Let _srcType_ be TypedArrayElementType(_source_). | ||
1. Let _srcElementSize_ be TypedArrayElementSize(_source_). | ||
1. Let _srcByteOffset_ be _source_.[[ByteOffset]]. | ||
1. If _targetOffset_ = +∞, throw a *RangeError* exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we intentionally separated out these comparisons to +∞ to avoid doing arithmetic on infinities, though it appears we do define it in §5.2.5 Mathematical Operations, so I guess it's fine. But I'd want the other editors to weigh in first.
spec.html
Outdated
1. If _length_ is *undefined*, then | ||
1. If _bufferByteLength_ modulo _elementSize_ ≠ 0, throw a *RangeError* exception. | ||
1. Let _newByteLength_ be _bufferByteLength_ - _offset_. | ||
1. If _newByteLength_ < 0, throw a *RangeError* exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you dropped this check for a negative _byteLength_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so... InitializeTypedArrayFromArrayBuffer's newByteLength is defined as bufferByteLength - offset, where bufferByteLength comes from ArrayBufferByteLength and offset comes from ToIndex. So this check throws a RangeError if and only if offset > bufferByteLength, which in the refactoring is covered right after defining bufferByteLength.
This should be most clear in the narrow commit: 72aa49e
The rest pretty much LGTM. This was a lot to review, and the similarity was pretty much only in that they change the same sections. I'd really prefer these get split into separate PRs. These aren't all-or-nothing: we may want to drop some commits entirely. It'd also help each piece land gradually instead of everything being blocked on everything getting reviewed/approved. |
I believe there's substantial overlap, e.g. ToElementCategory being used by TypedArray/DataView/Atomics, ValidateTypedArray being used by %ArrayIteratorPrototype%/TypedArray/Atomics, and IsSameBuffer being used by TypedArray/ArrayBuffer/SharedArrayBuffer (not to mention movement of updated operations like SetTypedArrayFromArrayLike and InitializeTypedArrayFromArrayBuffer and evolution of operations like SetTypedArrayFromTypedArray and TypedArray construction, which would invite merge conflicts if pulled out). I prefer to keep things together and am happy to perform any necessary git surgery (including removal of any rejected commits), but if carving things up will make review easier and/or accelerate merging, then along what dimensions would you like to see these commits separated? |
Any dimension, as long as either they make sense as an independent change or there is a series of small, logical steps that can be landed in order. I think the number of things happening in this PR (especially intermixed with rearranging AOs) will significantly slow us down here. Like, if you just opened some PRs that re-ordered some AOs, that could get 2 stamps within the hour and be out of our hair. |
|
…me [[ContentType]]" to "same [[TypedArrayName]]"
…t before ArrayBuffer writes
When not dealing with bigints, its output comes from ToNumber rather than ToIntegerOrInfinity, but is then always sent to NumericToRawBytes, and for Atomics operations always follows ValidateAtomicAccessOnIntegerTypedArray confirmation that the element type is an unclamped {INT,UINT}{8,16,32}. For those types, NumericToRawBytes encodes ℝ(_conversionOperation_(_value_)), where _conversionOperation_ is one of To{Int,Uint}{8,16,32}, all of which start by mapping NaN and zeros of either sign to +0 (corresponding with ToIntegerOrInfinity), additionally mapping infinities to +0 (harmlessly pulling forward work that was already happening to infinites preserved by ToIntegerOrInfinity anyway, because nothing touches the values in question between initial conversion and NumericToRawBytes), and truncation (corresponding with ToIntegerOrInfinity). So the behavior of NumericToRawBytes calls is not affected by this change, and the only special case is Atomics.store, which returns the output from ToIntegerOrInfinity specifically (e.g., a positive or negative infinity that was written into the buffer as 0).
…before [[TypedArrayName]] This groups together the "copy from source" cases (and their helper operations): * If called without arguments or with a number, allocate a new buffer of appropriate length via AllocateTypedArray(..., elementCount). * Else if called with an ArrayBuffer or SharedArrayBuffer, reuse it via InitializeTypedArrayFromArrayBuffer(O, buffer, byteOffset?, elementCount?). * Else if called with an iterable, allocate and populate a new buffer of appropriate length via InitializeTypedArrayFromList(O, values). * Else, assume an array-like and allocate and populate a new buffer of appropriate length via InitializeTypedArrayFromArrayLike(O, arrayLike).
…han the less-clear length
…igint constraints
5cfe42a
to
9773ff0
Compare
This is a series of editorial commits that strive to improve the consistency and comprehensibility of spec text relating to ArrayBuffers and APIs that build on top of them. There's more to come, but potentially much cleaner if built on top of #3619 (which is normative).