-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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
slices: add sorting and comparison functions #60091
Comments
CC @eliben |
These are the ones from x/exp/slices, which we've discussed at length before. The main blocker is how to spell 'cmp.Ordered'. For this discussion let's assume 'cmp.Ordered' is the spelling and not focus on that. Are there any concerns about the APIs otherwise? |
Sorry if this was explained elsewhere, but why isn't there a SortStable? |
@willfaught see the paper trail from #47619 (comment) |
Perhaps nitpicking, but the comment on Sort regarding NaNs seems a bit prescriptive considering that we now have math.Compare and math.Compare32. |
@zephyrtronium Just to be clear, are you suggesting this: // Use slices.SortFunc(x, func(a, b float64) bool {return math.Compare(a, b) < 0 }) |
More precisely, I am pointing out that the comment as written only says to use |
Thanks. Updated the initial comment. |
This proposal has been added to the active column of the proposals project |
@eliben This seems to be the relevant comment:
However, it doesn't link to the other comment it references. Can you summarize it? This seems to be saying that the Sort func already does a stable sort. If that's the case, then don't we need an unstable sort func as well? |
It refers to this comment: #47619 (comment) (GitHub's auto-hiding comments in long threads interferes with the bread crumbing). It has to do with what values |
I once again would like to advocate to use a single type of comparison function - i.e. to either make Having two flavors of comparison function means you have to implement both per type, or you have to spell out a wrapper every time you need to go from one to the other, or you have to have a generic function that can transform one into the other (and the latter two probably add function call overhead). ISTM that ~any use of |
FWIW this dual use is carried over from the So it seems like a bit of a conundrum; having just |
@eliben IMO adding a new package, with generics no less, frees us from this historical burden, if we choose so. |
Also, FWIW, |
I don't believe we should only provide the func versions. It happens frequently that you want to sort or search a slice by the usual < order provided by the language. Providing that directly without having to say ", cmp.Less" is more convenient and can be made far more efficient. This also follows other APIs in the language that provide a reasonable default behavior but then also have the extensible Func version, like strings.Index vs strings.IndexFunc. |
Along these same lines, after the discussion of min and max on #59488, we should probably provide slices.Min and slices.Max here, alongside slices.Sort, since Min and Max are the other natural operations that would use Ordered.
If these are invoked with an empty list, they |
What do they return when |
Updated: they panic. |
If we are going to add
Alternatively, maybe |
I'll look into adding these to |
@rsc
|
Change https://go.dev/cl/495195 mentions this issue: |
IMHO I don't object to adding Argmin/Argmax or |
Personally, I'd prefer argmin and argmax over min and max themselves:
It isn't hard to imagine that I'm in the minority camp, though. 🌝 |
For concreteness, I believe this is the API being proposed:
There was a typo in the result of CompareFunc (it had no result), which I've fixed here and above. |
@rsc there's a discussion in https://go-review.googlesource.com/c/exp/+/495195 about handling of Currently the
A straightforward implementation of This could also be a topic for the proposal review committee to consider. |
This argument isn't all that compelling anymore, either. The main complexity of implementing cmp functions is the switch needed to produce ternary results. But with the addition of |
Change https://go.dev/cl/496078 mentions this issue: |
Now that the `cmp` package exists, sorting and comparison functions from `x/exp/slices` can be ported to the standard library, using the `cmp.Ordered` type and the `cmp.Less` and `cmp.Compare` functions. This move also includes adjustments to the discussions in #60091 w.r.t. NaN handling and cmp vs. less functions, and adds Min/Max functions. The final API is taken from #60091 (comment) Updates #60091 Change-Id: Id7e6c88035b60d4ddd0c48dd82add8e8bc4e22d3 Reviewed-on: https://go-review.googlesource.com/c/go/+/496078 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Eli Bendersky <eliben@google.com> Run-TryBot: Eli Bendersky <eliben@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
No change in consensus, so accepted. 🎉 |
Implemented in https://go.dev/cl/496078 |
Change https://go.dev/cl/498175 mentions this issue: |
Updates #60091 Change-Id: I7438811f4e41a2977acbb5ac74c22a02c28c6597 Reviewed-on: https://go-review.googlesource.com/c/go/+/498175 Reviewed-by: Eli Bendersky <eliben@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Eli Bendersky <eliben@golang.org> Run-TryBot: Eli Bendersky <eliben@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com>
Change https://go.dev/cl/505796 mentions this issue: |
They should return the first of equal elements. No such clarification is required for Min/Max as for them equal elements are indistinguishable. For golang#60091 Change-Id: Iad58115d482add852c811e993131702b5b3bec5e Reviewed-on: https://go-review.googlesource.com/c/go/+/505796 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Eli Bendersky <eliben@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com>
They should return the first of equal elements. No such clarification is required for Min/Max as for them equal elements are indistinguishable. For golang#60091 Change-Id: Iad58115d482add852c811e993131702b5b3bec5e Reviewed-on: https://go-review.googlesource.com/c/go/+/505796 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Eli Bendersky <eliben@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com>
Now that the `cmp` package exists, sorting and comparison functions from `x/exp/slices` can be ported to the standard library, using the `cmp.Ordered` type and the `cmp.Less` and `cmp.Compare` functions. This move also includes adjustments to the discussions in golang#60091 w.r.t. NaN handling and cmp vs. less functions, and adds Min/Max functions. The final API is taken from golang#60091 (comment) Updates golang#60091 Change-Id: Id7e6c88035b60d4ddd0c48dd82add8e8bc4e22d3 Reviewed-on: https://go-review.googlesource.com/c/go/+/496078 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Eli Bendersky <eliben@google.com> Run-TryBot: Eli Bendersky <eliben@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
They should return the first of equal elements. No such clarification is required for Min/Max as for them equal elements are indistinguishable. For golang#60091 Change-Id: Iad58115d482add852c811e993131702b5b3bec5e Reviewed-on: https://go-review.googlesource.com/c/go/+/505796 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Eli Bendersky <eliben@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com>
Now that the `cmp` package exists, sorting and comparison functions from `x/exp/slices` can be ported to the standard library, using the `cmp.Ordered` type and the `cmp.Less` and `cmp.Compare` functions. This move also includes adjustments to the discussions in golang#60091 w.r.t. NaN handling and cmp vs. less functions, and adds Min/Max functions. The final API is taken from golang#60091 (comment) Updates golang#60091 Change-Id: Id7e6c88035b60d4ddd0c48dd82add8e8bc4e22d3 Reviewed-on: https://go-review.googlesource.com/c/go/+/496078 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Eli Bendersky <eliben@google.com> Run-TryBot: Eli Bendersky <eliben@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
They should return the first of equal elements. No such clarification is required for Min/Max as for them equal elements are indistinguishable. For golang#60091 Change-Id: Iad58115d482add852c811e993131702b5b3bec5e Reviewed-on: https://go-review.googlesource.com/c/go/+/505796 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Eli Bendersky <eliben@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com>
Update, May 18 2023: The current proposed API is #60091 (comment)
In #57433 we added the slices package to the standard library. In the discussion of that proposal, we decided to delay adding the sorting and comparison functions that exist in golang.org/x/exp/slices, pending a decision on the constraints package.
In #59488 we proposed adding a new cmp package, which will define
cmp.Ordered
as a constraint matching all ordered types.This proposal is to add the sorting functions to the slices package in the standard library, assuming that #59488 is adopted. If #59488 is declined, then this proposal should be declined.
The proposed new API (already in golang.org/x/exp/slices) is:
The text was updated successfully, but these errors were encountered: