Add a codegen test for comparisons of 2-tuples of primitives#108156
Closed
scottmcm wants to merge 1 commit intorust-lang:masterfrom
Closed
Add a codegen test for comparisons of 2-tuples of primitives#108156scottmcm wants to merge 1 commit intorust-lang:masterfrom
scottmcm wants to merge 1 commit intorust-lang:masterfrom
Conversation
Collaborator
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
scottmcm
commented
Feb 17, 2023
Member
Author
There was a problem hiding this comment.
Ah, interesting check_lt_via_cmp passed on LLVM 15, but not LLVM 14. Will bump the version needed.
EDIT: Done.
The operators are all overridden in full for tuples, so those parts pass easily, but they're worth pinning. Going via `Ord::cmp`, though, doesn't optimize away for anything but `cmp`+`is_le`. So this leaves `FIXME`s in the tests for the others.
scottmcm
force-pushed
the
tuple-cmp-codegen
branch
from
c127f33 to
dc37e37
Compare
Member
Author
|
Looks like I might as well just land this via #108157 instead; closing. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 5, 2023
…dtolnay Use `partial_cmp` to implement tuple `lt`/`le`/`ge`/`gt` In today's implementation, `(A, B)::gt` contains calls to *both* `A::eq` *and* `A::gt`. That's fine for primitives, but for things like `String`s it's kinda weird -- `(String, usize)::gt` has a call to both `bcmp` and `memcmp` (<https://rust.godbolt.org/z/7jbbPMesf>) because when `bcmp` says the `String`s aren't equal, it turns around and calls `memcmp` to find out which one's bigger. This PR changes the implementation to instead implement `(A, …, C, Z)::gt` using `A::partial_cmp`, `…::partial_cmp`, `C::partial_cmp`, and `Z::gt`. (And analogously for `lt`, `le`, and `ge`.) That way expensive comparisons don't need to be repeated. Technically this is an observable change on stable, so I've marked it `needs-fcp` + `T-libs-api` and will r? rust-lang/libs-api I'm hoping that this will be non-controversial, however, since it's very similar to the observable changes that were made to the derives (rust-lang#81384 rust-lang#98655) -- like those, this only changes behaviour if a type overrode behaviour in a way inconsistent with the rules for the various traits involved. (The first commit here is rust-lang#108156, adding the codegen test, which I used to make sure this doesn't regress behaviour for primitives.) Zulip conversation about this change: <https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/.60.3E.60.20on.20Tuples/near/328392927>.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[no compiler nor library changes]
The operators are all overridden in full for tuples, so those parts pass easily, but they're worth pinning.
Going via
Ord::cmp, though, doesn't optimize away for anything butcmp+is_le. So this leavesFIXMEs in the tests for the others, referencing #106107.(I was using this to test whether using Clang's IR pattern for C++20's
<=>optimized any better, so figured I might as well check it in for anyone else looking to investigate similar stuff.)