Editorial: Introduce definition for sorting according to lexicographic code unit order #3299
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bakkot
approved these changes
Mar 22, 2024
gibson042
approved these changes
Mar 22, 2024
spec.html
Outdated
| </dl> | ||
| <emu-alg> | ||
| 1. Let _result_ be a copy of _strings_. | ||
| 1. [declared="comparefn"] Sort _result_ into the same order as if an Array of the same values had been sorted using %Array.prototype.sort% with *undefined* as _comparefn_. |
There was a problem hiding this comment.
This is great, and I'd love to see it taken even further by building the methods on top of AOs rather than the other way around:
- Introduce new AO
SortList(_list_, _SortCompare_), replacing SortIndexedProperties step 4 with a call to it. - Introduce new AO
CompareStringsByCodeUnit(_x_, _y_), replacing CompareArrayElements step 7 onwards with a call to it (and possibly also updating IsLessThan to use it). - Redefine this SortStringListByCodeUnit AO to something like
1. Let _SortCompare_ be a new Abstract Closure with parameters (_x_, _y_) that performs the following steps when called: 1. Return CompareStringsByCodeUnit(_x_, _y_). 1. Return SortList(_strings_, _SortCompare_)
There was a problem hiding this comment.
That's deeper than I had hoped to dig with this PR; how strong is that preference? If there's broad support for that direction I'm happy to do it. But if it's likely to result in back and forth, I'd prefer to keep the smaller scope and open an issue to discuss further changes.
There was a problem hiding this comment.
I have no objection to pursuing that in a followup if it is contentious.
ljharb
pushed a commit
that referenced
this pull request
Mar 27, 2024
|
Editors discussed this and would prefer to just say something like "sorted according to lexicographic code unit order" in each of the places we need to do this. @syg specifically prefers that over introducing an AO which adds a layer of indirection. I don't think there's any ambiguity there. We can do that ourselves if you'd like, or of course you are welcome to do so here. |
|
Also, we may want to |
ptomato
force-pushed
the
sortstringlistbycodeunit
branch
from
4c190c8
to
4a93acb
Compare
|
Makes sense, done. I've also taken a stab at the |
bakkot
reviewed
Mar 28, 2024
ptomato
force-pushed
the
sortstringlistbycodeunit
branch
from
4a93acb
to
788a065
Compare
michaelficarra
approved these changes
Mar 28, 2024
ptomato
force-pushed
the
sortstringlistbycodeunit
branch
from
788a065
to
ff720a9
Compare
ptomato
changed the title
syg
approved these changes
Apr 24, 2024
bakkot
added
the
ready to merge
There are a few places where Lists of Strings are sorted with the awkward phrasing "ordered as if an Array of the same values had been sorted using %Array.prototype.sort% using *undefined* as _comparefn_." Change the wording of this, and add a definition in the section about List specification types. I find this clearer about the intention (the current language leaves it implicit that the sorting is by code unit), and more concise. This phrasing is used in several places in ECMA-402 as well, so this change will bring additional benefit there.
ljharb
force-pushed
the
sortstringlistbycodeunit
branch
from
ff720a9
to
ed82ebb
Compare
ptomato
added a commit
to tc39/proposal-temporal
that referenced
this pull request
Apr 30, 2024
No need for the extra AO SortStringListByCodeUnit. ECMA-262 editors decided to go with the wording "sorted according to lexicographic code unit order". The change to AvailableNamedTimeZoneIdentifiers has now been made directly in ECMA-262, so no need to carry that diff in the Temporal spec anymore. See tc39/ecma262#3299.
Ms2ger
pushed a commit
to tc39/proposal-temporal
that referenced
this pull request
May 2, 2024
No need for the extra AO SortStringListByCodeUnit. ECMA-262 editors decided to go with the wording "sorted according to lexicographic code unit order". The change to AvailableNamedTimeZoneIdentifiers has now been made directly in ECMA-262, so no need to carry that diff in the Temporal spec anymore. See tc39/ecma262#3299.
ptomato
added a commit
to ptomato/ecma402
that referenced
this pull request
May 7, 2024
See tc39/ecma262#3299. This is the language preferred by the editors.
ben-allen
pushed a commit
to tc39/ecma402
that referenced
this pull request
May 8, 2024
See tc39/ecma262#3299. This is the language preferred by the editors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a few places where Lists of Strings are sorted with the awkward phrasing "ordered as if an Array of the same values had been sorted using %Array.prototype.sort% using undefined as comparefn." Encapsulate this sorting into a new AO, SortStringListByCodeUnit. I find this clearer about the intention (the current language leaves it implicit that the sorting is by code unit), and more concise.
This phrasing is used in several places in ECMA-402 as well, so this change will bring additional benefit there.