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: Give TypedArraySortCompare its own sub-clause #2302

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Feb 10, 2021

(Salvaged from PR #1612.)

(GitHub's diff is not very helpful: most of the apparent diff arises from a change of indentation.)

@bakkot
Copy link
Contributor

bakkot commented Feb 10, 2021

You can suppress whitespace-only diffs by appending ?w=1, incidentally.

@claudepache
Copy link
Contributor

You can suppress whitespace-only diffs by appending ?w=1, incidentally.

Or, more visually:

Capture d’écran 2021-02-10 à 20 21 28

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

i like it

@ljharb ljharb requested review from michaelficarra, syg, bakkot and a team February 10, 2021 19:59
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

Sure, this is fine. I'd prefer to factor out the closed-over aliases to parameters in a future PR. Or possibly use a spec closure?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Feb 11, 2021

I'd prefer to factor out the closed-over aliases to parameters in a future PR. Or possibly use a spec closure?

(See issue #1884.)

I think it would be easiest to just add them as parameters.

On the other hand, if you want to factor out Array.p.sort and TypedArray.p.sort's commonalities into an abstract operation, then that would be easier if you recast SortCompare and TypedArraySortCompare as abstract closures that capture the extra aliases.

If you're interested in the latter, this PR would just be churn.

@ljharb
Copy link
Member

ljharb commented Feb 11, 2021

Let’s add them as params right now then :-) the closed-over things are icky - even if we still refactored the two later.

@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
@jmdyck jmdyck force-pushed the TypedArraySortCompare branch from 31d9ccd to bb18dcc Compare July 18, 2021 14:08
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

+1 to taking them as parameters.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 10, 2021

One problem with adding _comparefn_ and _buffer_ as parameters to TypedArraySortCompare (and presumably adding _comparefn_ to SortCompare likewise) is that it complicates the specification of the sorting post-condition and also the definition of 'consistent comparison function'. And not just that you need to add arguments to the invocations, but also that the added arguments aren't consistent between TypedArraySortCompare and SortCompare. There are ways to handle that, but I'd really rather not get into that in this PR (especially if #2305 will take care of it).

@michaelficarra
Copy link
Member

I'm fine with addressing it in a follow-up. Let's merge this as-is.

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jan 5, 2022
@ljharb ljharb force-pushed the TypedArraySortCompare branch from bb18dcc to 93183b8 Compare January 5, 2022 23:12
@ljharb ljharb merged commit 93183b8 into tc39:main Jan 5, 2022
@jmdyck jmdyck deleted the TypedArraySortCompare branch January 6, 2022 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants