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

proposal: x/exp/slices: add Sorted #56692

Closed
AMRALAA10 opened this issue Nov 10, 2022 · 14 comments
Closed

proposal: x/exp/slices: add Sorted #56692

AMRALAA10 opened this issue Nov 10, 2022 · 14 comments

Comments

@AMRALAA10
Copy link

AMRALAA10 commented Nov 10, 2022

It would be great to have a sorted, generic function similar to Sort but instead of sorting in place it returns a new sorted array.

func Sorted[E constraints.Ordered](x []E) []E

Usage:

// Instead of doing this
a := []int{ 7, 4, 6, 2, 9}
b := make([]int, len(a))
copy(b, a)
slices.Sort(b)

// do this
a := []int{ 7, 4, 6, 2, 9}
b := slices.Sorted(a)

In my opinion the second approach is cleaner, more readable and easier to understand.

@gopherbot gopherbot added this to the Proposal milestone Nov 10, 2022
@seankhliao
Copy link
Member

original would be

b := slices.Clone(a)
slices.Sort(b)

imo this is clear enough

@randall77
Copy link
Contributor

Sorted is a bit too close to sort.IsSorted, which has a different meaning.

@AMRALAA10
Copy link
Author

The name Sorted is inspired from python sorted function. it would be intuitive to many developers and they can guess what it does.

@AMRALAA10
Copy link
Author

AMRALAA10 commented Nov 10, 2022

original would be

b := slices.Clone(a)
slices.Sort(b)

imo this is clear enough

One line is clearer and more flixable than 3. consider this:

// instead of
a := functionReturnsArray()
slices.Sort(a)
passToFunction(a)

// can be reduced to 
 passToFunction(slices.Sorted(functionReturnsArray()))

@AMRALAA10
Copy link
Author

AMRALAA10 commented Nov 11, 2022

Another common scenario.
You want to compare 2 arrays without changing their order.

a_clone := slices.Clone(a)
b_clone := slices.Clone(b)

slices.Sort(a_clone)
slices.Sort(b_clone)

diff := cmp.Diff(a_clone, b_clone)

with Sorted

diff := cmp.Diff(slices.Sorted(a), slices.Sorted(b))

Think also if you want to compare more than 2.

@AMRALAA10 AMRALAA10 closed this as not planned Won't fix, can't repro, duplicate, stale Nov 12, 2022
@AMRALAA10
Copy link
Author

Seems like folks don't need this feature.

@ianlancetaylor
Copy link
Contributor

You can retract the proposal if you like, but note that the proposal process takes weeks even at its fastest. This proposal hadn't even made it to the committee. There are hundreds of proposals in the backlog, unfortunately; see https://github.com/orgs/golang/projects/17.

But I agree that the emoji voting on this proposal is not in favor.

@AMRALAA10
Copy link
Author

Will keep this open for now to give it complete shot, and the committee can decide.

@AMRALAA10 AMRALAA10 reopened this Nov 12, 2022
@rsc
Copy link
Contributor

rsc commented Nov 14, 2022

No one is arguing that slices.Sorted is not useful. The main problem from a Go API design standpoint is that it hides an allocation that the user has no control over, which makes programs that use slices.Sorted generate more garbage than they might otherwise appear to. In contrast, if you write slices.Clone first, then it's more obvious what is going on.

@AMRALAA10
Copy link
Author

Perfect, Thank you for explaining your point of view, Always appreciate a proper discussion :).

It is a good point and I agree with it, But also I don't think a function shouldn't be implemented just because it might not be obvious for some user, We could come up with better ways to make it obvious for them that this function allocates an extra space, those ways could be name changing, comments on the function, highlighting it on the docs, .. etc.

@icholy
Copy link

icholy commented Nov 15, 2022

The function could be called SortClone or CloneAndSort to make the allocation more obvious. You'd need to make a case that this comes up often enough to warrant a new function.

@rsc
Copy link
Contributor

rsc commented Nov 30, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Dec 7, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Dec 14, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Dec 14, 2022
@golang golang locked and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants