Skip to content
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

Editorial: Define SortCompare as an Abstract Closure #2606

Closed

Conversation

nicolo-ribaudo
Copy link
Member

This PR is extracted from a PR to the "Change Array by Copy" proposal (tc39/proposal-change-array-by-copy#69). Thanks @ljharb for pointing out that we can bring this improvement independently from that proposal.

The SortCompare AO implicitly receives _comparefn_ from Array.prototype.sort, in a way that looks like a closure (but I don't think that it is, since SortCompare is an AO and not an Abstract Closure defined in the Array.prototype.sort steps). This PR passes _comparefn_ explicitly as a parameter.

I also did the same thing for TypedArraySortCompare, that implicitly receives both _comparefn_ and _buffer_.

spec.html Outdated
<dt>description</dt>
<dd>It performs a numeric comparison rather than the string comparison used in <emu-xref href="#sec-array.prototype.sort"></emu-xref>.</dd>
</dl>
<emu-alg>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the next lines are just indented.

@nicolo-ribaudo nicolo-ribaudo changed the title Explicitly pass _comparefn_ to SortCompare Editorial: Explicitly pass _comparefn_ to SortCompare Jan 3, 2022
@nicolo-ribaudo nicolo-ribaudo force-pushed the sortcompare-comparefn-param branch from 3e14d34 to 9c212d8 Compare January 3, 2022 17:43
@ljharb
Copy link
Member

ljharb commented Jan 3, 2022

This helps progress on #1884.

spec.html Outdated
Comment on lines 39020 to 39021
_x_: a Number or BigInt,
_y_: a Number or BigInt,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_x_: a Number or BigInt,
_y_: a Number or BigInt,
_x_: a Number or a BigInt,
_y_: a Number or a BigInt,

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Jan 5, 2022
@ljharb ljharb requested review from syg, bakkot and a team January 5, 2022 23:07
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Jan 5, 2022
ljharb pushed a commit to nicolo-ribaudo/ecma262 that referenced this pull request Jan 5, 2022
@ljharb ljharb force-pushed the sortcompare-comparefn-param branch from 9c212d8 to 1698647 Compare January 5, 2022 23:19
@bakkot
Copy link
Contributor

bakkot commented Jan 5, 2022

This doesn't quite work as written because the spec talks about whether SortCompare is a consistent comparison function, which is a function of two parameters rather than 3.

The approach @jmdyck takes to resolving this in #2305 (which is blocked on, that PR has unintended normative consequences) is to make an abstract closure which captures the relevant parameters, so that we can talk about whether that abstract closure is a consistent comparison function without needing to change that definition. That seems like a good approach to take here while we work on #2305.

@nicolo-ribaudo nicolo-ribaudo force-pushed the sortcompare-comparefn-param branch from fee77ec to 05dc1a2 Compare February 16, 2022 18:08
@nicolo-ribaudo nicolo-ribaudo changed the title Editorial: Explicitly pass _comparefn_ to SortCompare Editorial: Define SortCompare as an Abstract Closure Feb 16, 2022
@@ -37544,7 +37544,22 @@ <h1>Array.prototype.sort ( _comparefn_ )</h1>
1. Append _kValue_ to _items_.
1. Set _k_ to _k_ + 1.
1. Let _itemCount_ be the number of elements in _items_.
1. [id="step-array-sort"] Sort _items_ using an implementation-defined sequence of calls to SortCompare. If any such call returns an abrupt completion, stop before performing any further calls to SortCompare or steps in this algorithm and return that completion.
1. [id="step-array-sort-sortcompare"] <a name="sec-sortcompare"></a> Let _sortCompare_ be a new Abstract Closure with parameters (_x_, _y_) that captures _comparefn_ and performs the following steps when called:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<a name="sec-sortcompare"></a> is so that existing links to the spec with #sec-sortcompare continue working:

  • Is that a goal we have?
  • Is there a better way to do it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen other sections with an oldids attribute, though not sure if that would work in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a goal we have?

Yes, but only weakly - we try to make old links keep working when we can point to roughly the place, but sometimes we can't and that's fine.

Is there a better way to do it?

oldids on the containing clause is probably the way to go. It won't point to exactly this step, but it's close enough.

@bakkot bakkot added the editor call to be discussed in the next editor call label Feb 16, 2022
@acutmore
Copy link
Contributor

Do we think we could split the difference and still retain the bulk of the steps as a seperate AO, reducing the steps in the abstract closure? That would make it easier to share with toSorted in https://github.com/tc39/proposal-change-array-by-copy.

Alternatively that could be done as part of the change-array-by-copy proposal itself.

@bakkot
Copy link
Contributor

bakkot commented Feb 17, 2022

I'm going to close this in favor of #2305, now that that PR has been updated to be strictly editorial. They're basically the same, except the older #2305 also refactors the definition of TypedArray.prototype.sort to be fully specified like other built-ins rather than with weird prose. I'm very sorry about the duplicated work. Please feel free to raise any comments there.

@acutmore: Yes, you could define a new AO (say CompareAsStrings) which is just the current body of the SortCompare abstract closure, and replace the current body of SortCompare with Return ? CompareAsStrings(_x_, _y_, _comparefn_).

@bakkot bakkot closed this Feb 17, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the sortcompare-comparefn-param branch February 17, 2022 15:39
@bakkot bakkot removed the editor call to be discussed in the next editor call label Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants