Various cosmetic improvements #3767
Conversation
|
reviewing these PRs takes a lot of time. The one on miri took me around an hour for the first round (it was slightly bigger to be fair). I will not be making this one a priority, I have little enough time for clippy as it is. If noone else has the time and takes this PR up for review in the next two weeks, I will close it to keep our queue free. |
|
Thanks, this looks like a welcome change in general! As Oli mentioned, reviewing big PRs takes time, which is something we don't have as much of in Clippy (AFAIK no one is getting paid to work on Clippy currently?) and as it stands currently, we would prioritize bug-fixes and new lints over this PR. To proceed, I would suggest to split this PR up into smaller ones to make it more easy for us to review these changes. (i.e., re-ordering I'm going to leave this open in case someone else wants to take on the review of this PR. However, please don't hesitate to open new smaller PRs in the meantime. |
|
Since I'm actually paid as a research assistant, I will pick this up and review it. 👍 |
There was a problem hiding this comment.
Some ui-tests are failing, since the line numbers changed:
failures:
[ui] ui/block_in_if_condition.rs
[ui] ui/doc.rs
[ui] ui/format.rs
[ui] ui/len_zero.rs
[ui] ui/lifetimes.rs
[ui] ui/methods.rs
Except for block_in_if_condition, running tests/ui/update-all-references.sh should be enough. Please make a separate commit for the stderr file updates for easier review.
| @@ -5,41 +5,48 @@ fn distinct_lifetimes<'a, 'b>(_x: &'a u8, _y: &'b u8, _z: u8) {} | |||
|
|
|||
| fn distinct_and_static<'a, 'b>(_x: &'a u8, _y: &'b u8, _z: &'static u8) {} | |||
|
|
|||
| fn same_lifetime_on_input<'a>(_x: &'a u8, _y: &'a u8) {} // no error, same lifetime on two params | |||
| // No error; same lifetime on two params. | |||
| fn same_lifetime_on_input<'a>(_x: &'a u8, _y: &'a u8) {} | |||
There was a problem hiding this comment.
These changes require a stderr update.
tests/ui/methods.rs
Outdated
| fn rem(self, other: T) { } // no error, wrong return type | ||
|
|
||
| fn into_u32(self) -> u32 { 0 } // fine | ||
| // No error; not public interface. |
There was a problem hiding this comment.
These changes require a stderr update
| format!("{:8}", "foo"); | ||
| format!("{:width$}", "foo", width = 8); | ||
| format!("{:+}", "foo"); // warn when the format makes no difference | ||
| format!("{:<}", "foo"); // warn when the format makes no difference | ||
| format!("{:+}", "foo"); // Warn when the format makes no difference. |
There was a problem hiding this comment.
These changes require a stderr update.
| @@ -63,7 +63,8 @@ impl PubTraitsToo for One { | |||
| } | |||
|
|
|||
| trait TraitsToo { | |||
| fn len(self: &Self) -> isize; // no error, len is private, see #1085 | |||
| fn len(self: &Self) -> isize; | |||
| // No error; `len` is private; see issue #1085. | |||
There was a problem hiding this comment.
These changes require a stderr update.
|
☔ The latest upstream changes (presumably #3789) made this pull request unmergeable. Please resolve the merge conflicts. |
phansch
added
the
S-waiting-on-author
|
Ping from triage @alexreg. Any updates on this? I saw that |
|
@flip1995 Sorry, completely forget about this! Will try to get to it tonight (or tomorrow at worst). |
|
@flip1995 Okay, I've resolved all the points in your review, except Thanks a lot for the review in any case! |
|
Thanks for all the work! Let's see if travis passes and then merge this. |
|
@flip1995 No problem. Appreciate you being open to these. Fingers crossed for Travis... |
|
Some doc tests in clippy_lints are failing. Adding a |
|
Thanks! |
|
@bors r+ p=10 |
|
📌 Commit 72aeaa8 has been approved by |
Various cosmetic improvements Related to the larger effort of rust-lang/rust#58036.
|
☀️ Test successful - checks-travis, status-appveyor |
Related to the larger effort of rust-lang/rust#58036.