-
Notifications
You must be signed in to change notification settings - Fork 20
Update sorting methods to use abstract closures #78
Conversation
@@ -18,6 +18,102 @@ <h1>Array Objects</h1> | |||
<emu-clause id="sec-properties-of-the-array-prototype-object"> | |||
<h1>Properties of the Array Prototype Object</h1> | |||
|
|||
<emu-clause id="sec-array.prototype.sort" oldids="sec-sortcompare"> |
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.
Array.prototype.sort
is updated to move most of the body of the _SortCompare_
inline abstract closure into a new AO that can be shared with toSorted
.
</emu-alg> | ||
</emu-clause> | ||
|
||
<emu-clause id="sec-sortindexedproperties" type="abstract operation"> |
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.
sortIndexedProperties
is updated to make it usable by toSorted
.
a) toSorted needs to write out to a new array, rather than mutate the original instance
b) toSorted ignores 'holes' in arrays, and will read them regardless.
@@ -197,6 +282,68 @@ <h1>The %TypedArray% Intrinsic Object</h1> | |||
<emu-clause id="sec-properties-of-the-%typedarrayprototype%-object"> | |||
<h1>Properties of the %TypedArray% Prototype Object</h1> | |||
|
|||
<emu-clause id="sec-%typedarray%.prototype.sort" oldids="sec-typedarraysortcompare"> |
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.
%typedarray%.prototype.sort
updated for same reason as Array above.
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
1. <del>Repeat, while _j_ < _itemCount_,</del> | ||
1. <del>Perform ? Set(_obj_, ! ToString(𝔽(_j_)), _items_[_j_], *true*).</del> | ||
1. <del>Set _j_ to _j_ + 1.</del> | ||
1. <del>Repeat, while _j_ < _len_,</del> |
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.
Nit: Add a note that this only happens when we respect holes.
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.
Good shout. Note added in Array.prototype.sort
where the delete prop loop was moved to.
1. Assert: Both Type(_x_) and Type(_y_) are Number or both are BigInt. | ||
1. If _comparefn_ is not *undefined*, then | ||
1. Let _v_ be ? ToNumber(? Call(_comparefn_, *undefined*, « _x_, _y_ »)). | ||
1. If IsDetachedBuffer(_buffer_) is *true*, throw a *TypeError* 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.
Does this need to be checked? Isn't it done implicitly with the Get
and Set
calls?
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.
Nope, Get and Set don't fail on detached buffers.
Also, this check happens after every call to comparefn
, whereas the Get and Set calls happen only before and after sorting the list.
[struck a false comment here, whoops]
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.
Good point. This check was there right from the ES6 version of TypedArray.prototype.sort
.
However until ES2021 sorting was defined as performing:
an implementation-dependent sequence of calls to the Get, Set ...
So the buffer could become detached before reading had finished. This was updated to match the current semantics in tc39/ecma262#1585
There were a lot of comments on that PR and maybe this was missed in amongst the noise?
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.
Yeah, probably.
Let's move discussion of what the behavior should be to tc39/ecma262#2646 (comment) (which is about a similar change to a different TA method). Changing the existing behavior is out of scope for this proposal, I imagine.
@ljharb @nicolo-ribaudo after you approved I renamed the
|
@acutmore i'm not sure that feels cleaner to me; it's not a huge deal because this isn't actual code, but all the existing callsites obviously omit this boolean, and will now be passing true instead of false. In a real API, you'd want omitting and false to always have the same behavior, ideally. |
Ah, as in this is like a breaking change for implementations that match the spec exactly. Yes I see that. My theory is that this AO was only added last week so there wouldn't be that many places that implement and call it yet. |
@acutmore sure, but it'll be in ES2022, and this proposal won't be until at least ES2023 :-) (obv this isn't going to be a motivation for most people, but es-abstract will thus be shipping the current version as 2022, and the breaking one as 202X) |
@ljharb good point that the change would cross versions! I have also changed the return type of the AO so I think call sites would likely need to be updated regardless? Im happy to flip the true/false logic back, was more about using the word “exclude” instead of “ignore”, as I kept forgetting if it was the hole that was being ignored (e.g read and allow prototype lookup) or indexes with holes being ignored (not reading when the property is absent). |
@acutmore true enough :-) it’s not a strong argument. I don’t particularly care about the naming, but i lightly prefer the default/false to mean “what sort does”. |
As it's only a light preference I'll stick with what we've got for now and merge this PR 😀 |
Updating
toSorted
now that tc39/ecma262#2305 has merged upstream.Replaces #69 and fixes #51
cc: @michaelficarra @bakkot @jmdyck