implement Ord for OutlivesPredicate and other types #50930
Merged
Conversation
This file contains 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
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@eddyb isolated the |
|
I think you can simply add that commit to #50070? |
eddyb
reviewed
May 21, 2018
|
|
||
| impl<'tcx> PartialOrd for TyS<'tcx> { | ||
| fn partial_cmp(&self, other: &TyS<'tcx>) -> Option<Ordering> { | ||
| Some(self.sty.cmp(&other.sty)) |
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
| fn partial_cmp(&self, other: &Slice<T>) -> Option<Ordering> { | ||
| if self == other { Some(Ordering::Equal) } else { | ||
| <[T] as PartialOrd>::partial_cmp(&self.0, &other.0) | ||
| } |
There was a problem hiding this comment.
Same here (Some(self.cmp(other))).
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?
There was a problem hiding this comment.
Ah, I somehow missed that this was generic - seems fine, then.
eddyb
reviewed
May 21, 2018
| @@ -578,6 +590,22 @@ impl <'gcx: 'tcx, 'tcx> Canonicalize<'gcx, 'tcx> for Ty<'tcx> { | |||
| #[derive(Debug, RustcEncodable)] | |||
| pub struct Slice<T>([T]); | |||
|
|
|||
| impl<T> Ord for Slice<T> where T: Ord { | |||
| fn cmp(&self, other: &Slice<T>) -> Ordering { | |||
| if self == other { Ordering::Equal } else { | |||
There was a problem hiding this comment.
credit goes to @nikomatsakis
eddyb
reviewed
May 21, 2018
| impl<'tcx> Ord for Kind<'tcx> { | ||
| fn cmp(&self, other: &Kind) -> Ordering { | ||
| match (self.unpack(), other.unpack()) { | ||
| (UnpackedKind::Type(_), UnpackedKind::Lifetime(_)) => Ordering::Greater, |
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.
There was a problem hiding this comment.
oops missed this one. Here is a pr for that #54074
|
@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
|
@nikomatsakis @eddyb rebased to master and should be ready for review |
nikomatsakis
approved these changes
May 21, 2018
|
@bors r+ |
|
📌 Commit fabe510 has been approved by |
bors
added
S-waiting-on-bors
|
☔ 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
|
@bors r+ |
|
📌 Commit 6e3a2ed has been approved by |
bors
added
S-waiting-on-bors
7 tasks
|
⌛ Testing commit 6e3a2ed7d3fd0173e507c3bf8f18bd416cdc640c with merge 4fc63c69d4512d3edc6b42624b34bba467b0ee29... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
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
|
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
|
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
|
@bors r+ |
|
📌 Commit 9a8400c has been approved by |
bors
added
S-waiting-on-bors
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.
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced May 24, 2018
Closed
Merged
eddyb
reviewed
Sep 8, 2018
| @@ -332,7 +332,7 @@ impl AddAssign for Size { | |||
| /// Each field is a power of two, giving the alignment a maximum value | |||
| /// 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)] | |||
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.