-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Editorial: Make better use of ValidateTypedArray #3634
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
1. If _array_ has a [[TypedArrayName]] internal slot, then | ||
1. Let _taRecord_ be MakeTypedArrayWithBufferWitnessRecord(_array_, ~seq-cst~). | ||
1. If IsTypedArrayOutOfBounds(_taRecord_) is *true*, throw a *TypeError* exception. | ||
1. Let _taRecord_ be ? ValidateTypedArray(_array_, ~seq-cst~). |
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.
This seems worse to me. Every existing use of ValidateTypedArray is passed an object which is not yet known to be a TA.
I'd be OK with introducing a new AO for this - GetInBoundsTALengthWitness or something, maybe - and then using it ValidateTypedArray and in callsites such as this. But I really don't like re-using ValidateTypedArray for this.
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 agree with Kevin's preferred approach over the current one in the PR. As a general thing I prefer AO preconditions to never be superfluous at callsites. If there are superfluous checks, a refactoring of the kind Kevin laid out is best for clarity, so as not to raise questions for future readers.
Edit: To be clearer, the questions I'm interested in not raising for future readers are like "why is this AO called here if it doesn't need to check these conditions?" (because of the expectation that the spec strives to be as "exact" in its verbiage as it can be) and if the answer after spec archaeology is "because it was convenient" I think that's a net negative on the readability.
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.
Why remove the else
guard? There's no reason to do this check when _waitable_
is *true*
, since it is implied by the checks in that branch.
Split from #3629 as requested at #3629 (comment)