Rustup #3889
Rustup #3889
Conversation
flip1995
added
E-help-wanted
|
cc @eddyb |
|
Ok thanks for the review everyone! Now I just have to fix the tests. |
flip1995
added
S-waiting-on-review
|
Yet another breakage rust-lang/rust#58899 Very very WIP: mati865@d3447a2 EDIT2: Further WIP https://github.com/mati865/rust-clippy/tree/rustup |
| fn match_borrow_depth(lhs: &ty::TyKind<'_>, rhs: &ty::TyKind<'_>) -> bool { | ||
| match (lhs, rhs) { | ||
| (ty::Ref(_, t1, _), ty::Ref(_, t2, _)) => match_borrow_depth(&t1.sty, &t2.sty), | ||
| fn match_borrow_depth(lhs: ty::Ty<'_>, rhs: ty::Ty<'_>) -> bool { |
There was a problem hiding this comment.
You should import Ty and always refer to it unqualified - that's at least the rustc convention.
| ABSOLUTE | ||
| use rustc::ty::print::Printer; | ||
|
|
||
| #[allow(clippy::diverging_sub_expression)] |
There was a problem hiding this comment.
This sounds like a false positive - is an issue filled?
There was a problem hiding this comment.
Not really a FP:
Since type Error = ! and we call let mut path = print_prefix(self)?;, the ? is a diverging sub-expression. IMO allowing this here is ok.
There was a problem hiding this comment.
I disagree, ? with a diverging error path should not be warned against, the behavior is more like that of let Ok(mut path) = print_prefix(self);.
| pub const PARTIAL_ORD: [&str; 3] = ["core", "cmp", "PartialOrd"]; | ||
| pub const PATH: [&str; 3] = ["std", "path", "Path"]; | ||
| pub const PATH_BUF: [&str; 3] = ["std", "path", "PathBuf"]; | ||
| pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"]; | ||
| pub const PATH_TO_PATH_BUF: [&str; 2] = ["<std::path::Path>", "to_path_buf"]; |
There was a problem hiding this comment.
These don't look good... That means path_qualified isn't quite right.
You want at least this, at the start of path_qualified:
if trait_ref.is_none() {
if let ty::Adt(def, substs) = self_ty.sty {
return self.print_def_path(def.did, substs);
}
}There was a problem hiding this comment.
Yeah that would be better. Should I also open a rustc PR to also change it there?
There was a problem hiding this comment.
Where? rustdoc? Maybe, I don't know what that's used for (cc @QuietMisdreavus)
| (ty::Bool, ty::Bool) | ||
| | (ty::Char, ty::Char) | ||
| | (ty::Int(_), ty::Int(_)) | ||
| | (ty::Uint(_), ty::Uint(_)) | ||
| | (ty::Str, ty::Str) => true, | ||
| (ty::Ref(_, t1, _), ty::Ref(_, t2, _)) | ||
| | (ty::Array(t1, _), ty::Array(t2, _)) | ||
| | (ty::Slice(t1), ty::Slice(t2)) => match_types(&t1.sty, &t2.sty), | ||
| | (ty::Slice(t1), ty::Slice(t2)) => match_types(&t1, &t2), |
There was a problem hiding this comment.
&x.field, when x: &T, is &(*x).field, so when you want the whole thing instead of the field, you can write just x (&x happens to work, but it's because deref coercions stripping the &).
| fn match_types(lhs: &ty::TyKind<'_>, rhs: &ty::TyKind<'_>) -> bool { | ||
| match (lhs, rhs) { | ||
| fn match_types(lhs: ty::Ty<'_>, rhs: ty::Ty<'_>) -> bool { | ||
| match (&lhs.sty, &rhs.sty) { |
There was a problem hiding this comment.
Also, random note, cc @Manishearth @oli-obk: this looks like it could use the ty::relate infrastructure.
|
Thanks for the review! I won't be able to get to this until this evening. @mati865 if you want to open a new rustup PR based on this, feel free to do so! |
|
New PR: #3893 |
Rustup Supersedes #3889 Addresses some review comments from previous PR and rustups to rust-lang/rust#58899
Rustup Supersedes #3889 Addresses some review comments from previous PR and rustups to rust-lang/rust#58899
Rustup Supersedes #3889 Addresses some review comments from previous PR and rustups to rust-lang/rust#58899
Rustup Supersedes #3889 Addresses some review comments from previous PR and rustups to rust-lang/rust#58899
Rustup Supersedes #3889 Addresses some review comments from previous PR and rustups to rust-lang/rust#58899
This is a WIP of the rustup to rust-lang/rust#58140. I'll try to continue working on this tomorrow. In the mean time everyone can pick this up if someone wants to :)