-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
cmp: new package with Ordered, Compare, Less; min, max as builtins #59488
Comments
These seem like good utilities to have in the standard library, and I think Did you consider making func Min[T Ordered](vals ...T) T
func Max[T Ordered](vals ...T) T It would of course be possible to implement the variadic versions in terms of the two-argument versions in my own code if the standard library didn't offer a variadic version, so this is perhaps superflous, but since the variadic version could still be used with two arguments it doesn't seem like it loses any functionality to make it variadic. (Perhaps there is a performance cost compared to just regular arguments? Is it significant?) |
It sounds like |
When T is a float, we will need to describe behavior in all the special cases (NaN, +/- 0). |
Is there any case for the |
Floats are certainly tricky. But most users probably don't hit and/or care about those weird cases. So from an ergonomic API view, allowing floats makes sense. It just complicates the implementation a bit, and requires a bit of extra doc for those who care. |
I lean towards putting these in a separate Also, There is no harm in putting these things in a separate package, and the possibility of appreciable good accruing. |
I suppose this question about floats boils down to specifying how It will presumably then need to make an essentially-arbitrary decision, but that decision ought to be subject to Go's compatibility promises. Is there a typical answer to this that users might expect from other languages, or is this literally a case of "just pick a behavior and document it"? |
@apparentlymart In the general case variadic functions will be more expensive, because they require constructing a slice. Of course in general the functions will likely be inlined and the cost will disappear. But, especially since almost all existing examples I found take just two arguments, I think it's preferable to have the two argument version. We can consider adding variadic versions separately. I'm open to counter-argument.
@randall77 I think the suggested documentation is clear about the behavior for ±0. I suppose we may have to document what happens for NaN. Sigh. I think my preference would be to say that if either value is a |
I don't feel strongly about them being variadic; I was considering it only because it seemed like a more general form that could therefore be more broadly useful without losing the convenience of the two-argument case. However, the fact that the other examples you found are not variadic does indeed seem to suggest that it's not necessary, and combining with the possible performance cost does seem to make the two-argument form seem like the better choice. A variadic version would be pretty simple to implement elsewhere when needed. |
As far as I know, the only way to implement this is via If we had something like #45380 it would undoubtedly be simple, but that's still on ice. Personally, I neither like the idea of adding a constraint that excludes floats, nor do I like the idea of adding an imprecise implementation for floats to the stdlib, nor do I like the idea of using |
Extrapolating a bit, if a builtin Ordered constraint also accepted types with some |
@fgm The standard library is converging on exporting two APIs to support "native" and "custom" operators. e.g. having slices.Equal and slices.EqualFunc. By that approach, we'd have to have For what it's worth, in my opinion there just is no unambiguously good way to support both "native" and "custom" operators. There are alternatives to the "two APIs" approach, but they have their own problems. |
One question, is it possible make And, as rob mentioned, |
@Merovius I think we can make the implementation fast and general. For example:
The first line there will completely compile away for non-float types. |
I'd rather like to see the |
So, I have conflicting desires:
|
|
Note that a new cmp package was previously proposed here: #58559 (comment) |
I lean towards having cmp.TotalOrdered which excludes floats and having a comment telling users to use math.Min/Max/Compare if they need floats. If someone really needs a generic sort that includes fast float missorting they can write it themselves trivially. |
@fzipp
Proposal #45200 for "testing/cmp" could be changed to "testing/equal" or "testing/diff" or something else. It would require reflection, so it should be kept separate from the "cmp" package that is discussed here. |
variadic Min/Max has some weird corners. You want to keep the property that That probably seems academic but consider this innocuous line of code This has two consequences. There needs to be some way for these to get the greatest/least element of an arbitrary You could have them panic when they receive an empty slice or make the signature something like Binary Min/Max is simpler. Per @neild's suggestion, a |
There is some implicit conventional usage of edit: From the example of |
I like ord, but it should be ord.Compare (not Cmp) to match strings.Compare and math.Compare. |
If Go will supported some ordered types which are hard to be described in type constraints, then it would better to add a builtin |
@go101 Yes, if Go ever supports ordered struct or array types, then it will be necessary to do something like define |
I'm not personally excited about a package name "ord", but I could see |
For golang/go#59488 Change-Id: I43d9a5b644a9c3ce647a11f9e2b647093b070c9f Reviewed-on: https://go-review.googlesource.com/c/tools/+/498515 Reviewed-by: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> gopls-CI: kokoro <noreply+kokoro@google.com> Reviewed-by: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Updates #59488 Change-Id: If873b81fb7f0e28b84a3e5c2ff89426b3e289d5d Reviewed-on: https://go-review.googlesource.com/c/go/+/498495 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
For golang/go#59488 Change-Id: I93680138c90750454b4d94af6dc84fe942c9dd34 Reviewed-on: https://go-review.googlesource.com/c/tools/+/498516 Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Robert Findley <rfindley@google.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com> gopls-CI: kokoro <noreply+kokoro@google.com>
Change https://go.dev/cl/501355 mentions this issue: |
Change https://go.dev/cl/501697 mentions this issue: |
For #59488 Change-Id: I73ee4d1d8b9d8e6f0aad9e3bb98729aaa0f06a47 Reviewed-on: https://go-review.googlesource.com/c/go/+/501697 TryBot-Bypass: Ian Lance Taylor <iant@google.com> Reviewed-by: Robert Griesemer <gri@google.com> Reviewed-by: Eli Bendersky <eliben@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Auto-Submit: Ian Lance Taylor <iant@google.com>
Change https://go.dev/cl/501825 mentions this issue: |
For #59488. For #58645. Change-Id: Ia9b76d49825dd74f7e52d829ec6d47e6c2addd76 Reviewed-on: https://go-review.googlesource.com/c/go/+/501825 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Bypass: Dmitri Shuralyov <dmitshur@google.com>
For #59488 Fixes #60616 Change-Id: Idf9f42d7d868999664652dd7b478684a474f1d96 Reviewed-on: https://go-review.googlesource.com/c/go/+/501355 Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Rob Pike <r@golang.org> Run-TryBot: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Ian Lance Taylor <iant@golang.org>
Double-check something: @rsc wrote:
In testing this out with the Go playground with gotip, https://go.dev/play/p/v-F_sKZp62W?v=gotip Was this a deliberate change in behavior, or is it a mistake to be fixed before 1.21 final? |
@philpennock It was a deliberate change based on further consideration of implementation considerations. See #59488 (comment) . We decided that if any argument is a NaN, the result would be a NaN, but we aren't specifying the exact NaN value. This seems consistent with operations like addition of two NaN values. |
Ah, sorry, it wasn't clear that was changing the proposed contract of returning the earliest, but looking now, I see that reading in there. Thank you. |
It was a good question, we kind of skated over that during the implementation. At least I think the docs are correct. |
Change https://go.dev/cl/514596 mentions this issue: |
Change https://go.dev/cl/514775 mentions this issue: |
Update #59488 Change-Id: I89f5ea494cbcc887f6fae8560e57bcbd8749be86 Reviewed-on: https://go-review.googlesource.com/c/go/+/514596 Reviewed-by: Keith Randall <khr@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com>
CL 514596 adds float min/max for amd64, this CL adds it for riscv64. The behavior of the RISC-V FMIN/FMAX instructions almost match Go's requirements. However according to RISCV spec 8.3 "NaN Generation and Propagation" >> if at least one input is a signaling NaN, or if both inputs are quiet >> NaNs, the result is the canonical NaN. If one operand is a quiet NaN >> and the other is not a NaN, the result is the non-NaN operand. Go using quiet NaN as NaN and according to Go spec >> if any argument is a NaN, the result is a NaN This requires the float min/max implementation to check whether one of operand is qNaN before float mix/max actually execute. This CL also fix a typo in minmax test. Benchmark on Visionfive2 goos: linux goarch: riscv64 pkg: runtime │ float_minmax.old.bench │ float_minmax.new.bench │ │ sec/op │ sec/op vs base │ MinFloat 158.20n ± 0% 28.13n ± 0% -82.22% (p=0.000 n=10) MaxFloat 158.10n ± 0% 28.12n ± 0% -82.21% (p=0.000 n=10) geomean 158.1n 28.12n -82.22% Update #59488 Change-Id: Iab48be6d32b8882044fb8c821438ca8840e5493d Reviewed-on: https://go-review.googlesource.com/c/go/+/514775 Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com> Run-TryBot: M Zhuo <mengzhuo1203@gmail.com> Reviewed-by: Joel Sing <joel@sing.id.au> Reviewed-by: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Keith Randall <khr@google.com>
CL 514596 adds float min/max for amd64, this CL adds it for riscv64. The behavior of the RISC-V FMIN/FMAX instructions almost match Go's requirements. However according to RISCV spec 8.3 "NaN Generation and Propagation" >> if at least one input is a signaling NaN, or if both inputs are quiet >> NaNs, the result is the canonical NaN. If one operand is a quiet NaN >> and the other is not a NaN, the result is the non-NaN operand. Go using quiet NaN as NaN and according to Go spec >> if any argument is a NaN, the result is a NaN This requires the float min/max implementation to check whether one of operand is qNaN before float mix/max actually execute. This CL also fix a typo in minmax test. Benchmark on Visionfive2 goos: linux goarch: riscv64 pkg: runtime │ float_minmax.old.bench │ float_minmax.new.bench │ │ sec/op │ sec/op vs base │ MinFloat 158.20n ± 0% 28.13n ± 0% -82.22% (p=0.000 n=10) MaxFloat 158.10n ± 0% 28.12n ± 0% -82.21% (p=0.000 n=10) geomean 158.1n 28.12n -82.22% Update golang#59488 Change-Id: Iab48be6d32b8882044fb8c821438ca8840e5493d Reviewed-on: https://go-review.googlesource.com/c/go/+/514775 Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com> Run-TryBot: M Zhuo <mengzhuo1203@gmail.com> Reviewed-by: Joel Sing <joel@sing.id.au> Reviewed-by: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Keith Randall <khr@google.com>
Update, May 17 2023: Current proposal at #59488 (comment).
(Edited to change the package from sort to cmp).
I propose adding a new package cmp, which will initially define the following:
The
Min
andMax
functions are trivial but are widely used. The standard library contains 14 versions ofMin
and 7 versions ofMax
(not countingmath.Min
andmath.Max
). With such wide usage it's worth providing versions in the standard library.The
Ordered
constraint is useful forMin
andMax
and also for sorting functions such as those in the x/exp/slices package. This would replaceconstraints.Ordered
in the x/exp/constraints package. GitHub code search reports thatconstraints.Ordered
appears in 2300 files in GitHub, so people do find it useful.If we adopt this proposal, we would not add a constraints package to the standard library.
Ordered
,Min
, andMax
are related to comparing values. Therefore, a package cmp is a reasonable location for them. We could also add them to the existing package sort.Possible other additions to a cmp package would be names for the values returned by
bytes.Compare
and similar functions, such ascmp.Less
(-1
),cmp.Eq
(0
), andcmp.Greater
(1
). We could also add a function wrappers that reverses the sense of a less-than comparator, and one that reverses the sense of abyte.Compare
style comparator.We should define how
Min
andMax
treat NaN values when instantiated with a floating-point type. I propose that if the first argument is a NaN then it will be returned, otherwise if the second argument is a NaN then it will be returned, otherwise compare with<
. When comparing a negative zero and a positive zero, I propose that the first argument will be returned, as with equal values; that is bothMin(0.0, math.Copysign(0.0, -1))
andMin(math.Copysign(0.0, -1), 0.0)
will return0
.Other languages
C++ puts
std::min
andstd::max
in<algorithm>
. Go's standard library doesn't have anything comparable. The C++<algorithm>
header is a bit of a catch-all, including functions that Go has insort
andcontainer/heap
, and functions that Go might put into a possibleiter
package in the future.Java has
Math.min
andMath.max
, which are overloaded functions that takedouble
,float
,int
, orlong
arguments. Go already hasmath.Min
andmath.Max
, but since Go doesn't have overloading they only takefloat64
arguments. Most functions in Go's match package are most naturally defined for floating-point values, so even generic versions of them might not take all ordered values. Andmath.Ordered
doesn't seem right.Python has
min
andmax
builtin functions. There's no particular reasons to make these functions predeclared in Go, since we can write them directly in Go.Rust has
std::cmp::min
andstd::cmp::max
.std::cmp
also has the Rust equivalents ofcomparable
andOrdered
, and some support for comparator functions. This offers some support for using a cmp package in Go. On the other hand Rust's standard library has slice/vector sorting, and has various sorted container types, but as far as I know (which isn't far) doesn't have a general purpose sorting algorithm like the one in Go's sort package.The text was updated successfully, but these errors were encountered: