implement Ord for OutlivesPredicate and other types#50930
Merged
bors merged 1 commit intorust-lang:masterfrom May 24, 2018
Merged
implement Ord for OutlivesPredicate and other types#50930bors merged 1 commit intorust-lang:masterfrom
bors merged 1 commit intorust-lang:masterfrom
Conversation
Contributor
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Contributor
Author
Contributor
Author
|
@eddyb isolated the |
Contributor
|
I think you can simply add that commit to #50070? |
eddyb
reviewed
May 21, 2018
src/librustc/ty/mod.rs
Outdated
Member
There was a problem hiding this comment.
This could be Some(self.cmp(other)), to keep the implementation details in the Ord impl.
eddyb
reviewed
May 21, 2018
src/librustc/ty/mod.rs
Outdated
Member
There was a problem hiding this comment.
Same here (Some(self.cmp(other))).
Contributor
Author
There was a problem hiding this comment.
Some(self.cmp(other)) will require the bound T: Ord, which I was not sure we wanted to impose for all types that that maybe only want to implement PartialOrd?
Member
There was a problem hiding this comment.
Ah, I somehow missed that this was generic - seems fine, then.
eddyb
reviewed
May 21, 2018
src/librustc/ty/mod.rs
Outdated
eddyb
reviewed
May 21, 2018
src/librustc/ty/subst.rs
Outdated
Member
There was a problem hiding this comment.
You don't need to implement all of this manually, just add #[derive(PartialEq, Eq, PartialOrd, Ord)] on UnpackedKind and implement this method going through that. That's how other traits are implemented on Kind.
Contributor
Author
There was a problem hiding this comment.
oops missed this one. Here is a pr for that #54074
Contributor
Author
|
@ishitatsuyuki A reason for extracting this into a separate PR was because the changes in this PR are isolated from #50070 and are useful on their own. Also this way others not familiar with #50070 can review these changes without prior context and hopefully its less messy and easier to understand :) |
toidiu
force-pushed
the
ak-ordOutlivesPredicate
branch
from
c3c8049 to
fabe510
Compare
Contributor
Author
|
@nikomatsakis @eddyb rebased to master and should be ready for review |
nikomatsakis
approved these changes
May 21, 2018
Contributor
|
@bors r+ |
Collaborator
|
📌 Commit fabe510 has been approved by |
bors
added
S-waiting-on-bors
Collaborator
|
☔ The latest upstream changes (presumably #50520) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
toidiu
force-pushed
the
ak-ordOutlivesPredicate
branch
from
fabe510 to
6e3a2ed
Compare
Contributor
|
@bors r+ |
Collaborator
|
📌 Commit 6e3a2ed has been approved by |
bors
added
S-waiting-on-bors
7 tasks
Collaborator
|
⌛ Testing commit 6e3a2ed7d3fd0173e507c3bf8f18bd416cdc640c with merge 4fc63c69d4512d3edc6b42624b34bba467b0ee29... |
Collaborator
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
toidiu
force-pushed
the
ak-ordOutlivesPredicate
branch
from
6e3a2ed to
1a159f6
Compare
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
toidiu
force-pushed
the
ak-ordOutlivesPredicate
branch
from
1a159f6 to
9a8400c
Compare
Contributor
|
@bors r+ |
Collaborator
|
📌 Commit 9a8400c has been approved by |
bors
added
S-waiting-on-bors
Collaborator
bors
added a commit
that referenced
this pull request
May 24, 2018
implement Ord for OutlivesPredicate and other types It became necessary while implementing #50070 to have `Ord` implemented for `OutlivesPredicate`. This PR implements `Ord` for `OutlivesPredicate` as well as other types needed for the implementation.
Collaborator
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced May 24, 2018
Closed
Merged
eddyb
reviewed
Sep 8, 2018
| /// of 2<sup>(2<sup>8</sup> - 1)</sup>, which is limited by LLVM to a | ||
| /// maximum capacity of 2<sup>29</sup> or 536870912. | ||
| #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)] | ||
| #[derive(Copy, Clone, PartialEq, Eq, Ord, PartialOrd, Hash, Debug, RustcEncodable, RustcDecodable)] |
Member
There was a problem hiding this comment.
This is a mistake, because this type does not have a defined ordering (see the min/max methods).
I'll try to figure out if I can fix it locally (as part of a PR that would probably remove the need for using this type in most places, by replacing it with one alignment, instead of two).
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
Sep 12, 2018
simplify ordering for Kind Missed from rust-lang#50930 r? @eddyb
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
7 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.
It became necessary while implementing #50070 to have
Ordimplemented forOutlivesPredicate.This PR implements
OrdforOutlivesPredicateas well as other types needed for the implementation.