Cleanup field sorting #77
Conversation
Use of cmp.Or with a series of compare function is more readable as it clearly which ordering dimension takes precedence.
| return 0 // TODO(https://go.dev/issue/61643): Compare bools better. | ||
| } | ||
| slices.SortStableFunc(allFields, func(x, y structField) int { | ||
| return cmp.Or( |
There was a problem hiding this comment.
Sorting multiple dimensions has now become my favorite use of cmp.Or
There was a problem hiding this comment.
Wow never thought of this
There was a problem hiding this comment.
in this particular case it doesn't seem to be a big deal, but isn't the use of cmp.Or causing all arguments to all be computed upfront? in the old implementation, if e.g. the names didn't match, then you didn't have to run cmp.Compare on the index lengths.
again in this case I doubt it's a noticeable difference, but if you have a cmp.Or with five chained string comparisons, I bet it can matter if often it's the first comparison that would make the other ones unnecessary.
There was a problem hiding this comment.
This only happens once per type and then the result is cached. I'll take readability over performance in this case.
| @@ -544,3 +536,15 @@ func consumeTagOption(in string) (string, int, error) { | |||
| func isLetterOrDigit(r rune) bool { | |||
| return r == '_' || unicode.IsLetter(r) || unicode.IsNumber(r) | |||
| } | |||
|
|
|||
| // boolsCompare compares x and y, ordering false before true. | |||
| func boolsCompare(x, y bool) int { | |||
There was a problem hiding this comment.
I'm still sad the stdlib or language still doesn't provide a first-class way to compare bools.
At Tailscale we added a package just for this: https://pkg.go.dev/tailscale.com/types/bools#Compare
| @@ -544,3 +536,15 @@ func consumeTagOption(in string) (string, int, error) { | |||
| func isLetterOrDigit(r rune) bool { | |||
| return r == '_' || unicode.IsLetter(r) || unicode.IsNumber(r) | |||
| } | |||
|
|
|||
| // boolsCompare compares x and y, ordering false before true. | |||
There was a problem hiding this comment.
Is it worth keeping the reference to https://go.dev/issue/61643 here so we don't forget that there is a declined proposal for this?
There was a problem hiding this comment.
The challenge is there's been so many bool-to-int or compare-bools proposals. 61643 isn't exactly unique. I think a standalone helper that is equivalent to bools.Compare will be useful for static analysis to one-day prove how often this is used rather than a reference to the issue.
There was a problem hiding this comment.
It's okay, I'm sure the next bool-to-int proposal will get approved 😉
Use of cmp.Or with a series of compare function is more readable as it clearly which ordering dimension takes precedence.