100% Clippy #1112
100% Clippy #1112
Conversation
Nemo157
force-pushed
the
clippy
branch
3 times, most recently
from
f67a25a to
144acf9
Compare
| @@ -205,6 +205,12 @@ impl LocalPool { | |||
| } | |||
| } | |||
|
|
|||
| impl Default for LocalPool { | |||
There was a problem hiding this comment.
I could see arguments for not implementing Default for LocalPool, if so I think having an explicit allow for it is good documentation that it has been considered and decided against.
There was a problem hiding this comment.
I can't think of any such arguments ^^' Could you list some?
There was a problem hiding this comment.
Mostly that a LocalPool isn’t a data structure, so being able to create a default in a generic context isn’t that useful. IMO it’s still worth having the impl anyway though.
futures-util/src/sink/with.rs
Outdated
| @@ -58,7 +58,8 @@ enum State<Fut, T> { | |||
| } | |||
|
|
|||
| impl<Fut, T> State<Fut, T> { | |||
| fn as_pin_mut<'a>( | |||
| #[allow(needless_lifetimes, wrong_self_convention)] // https://github.com/rust-lang/rfcs/pull/2362#issuecomment-406804183 | |||
| fn as_pin_mut( | |||
| self: PinMut<'a, Self> | |||
There was a problem hiding this comment.
missing trailing comma
|
I'm in favor of merging this. We can always remove it again from the CI if it proves to be a hassle. However, I'm optimistic that we'll benefit from having it in there. |
futures-util/src/io/mod.rs
Outdated
| @@ -288,7 +288,7 @@ pub trait AsyncWriteExt: AsyncWrite { | |||
| /// assert_eq!(output, [1, 2, 3, 4, 0]); | |||
| /// # Ok::<(), Box<std::error::Error>>(()) }).unwrap(); | |||
| /// ``` | |||
| fn write_all<'a>(&'a mut self, buf: &'a [u8]) -> WriteAll<'a, Self> { | |||
| fn write_all(&'a mut self, buf: &'a [u8]) -> WriteAll<'a, Self> { | |||
There was a problem hiding this comment.
Was it intentional to use in-band-lifetimes here?
There was a problem hiding this comment.
Yep, as far as I’m aware they’re still on track to be stable in 2018 Edition. If you’ve got some reason not to use them I can revert these changes.
There was a problem hiding this comment.
@cramertj My local waker opt PR you just merged also had an in-band lifetime in it 😇
| @@ -212,7 +212,7 @@ pub trait SinkExt: Sink { | |||
| /// Doing `sink.send_all(stream)` is roughly equivalent to | |||
| /// `stream.forward(sink)`. The returned future will exhaust all items from | |||
| /// `stream` and send them to `self`. | |||
| fn send_all<'a, St>( | |||
| fn send_all<St>( | |||
futures-util/src/sink/with.rs
Outdated
| @@ -58,7 +58,8 @@ enum State<Fut, T> { | |||
| } | |||
|
|
|||
| impl<Fut, T> State<Fut, T> { | |||
| fn as_pin_mut<'a>( | |||
| #[allow(needless_lifetimes, wrong_self_convention)] // https://github.com/rust-lang/rfcs/pull/2362#issuecomment-406804183 | |||
| fn as_pin_mut( | |||
| @@ -60,7 +60,8 @@ where | |||
| } | |||
|
|
|||
| /// Get a pinned mutable reference to the inner sink. | |||
| pub fn get_pin_mut<'a>(self: PinMut<'a, Self>) -> PinMut<'a, Si> { | |||
| #[allow(needless_lifetimes)] // https://github.com/rust-lang/rfcs/pull/2362#issuecomment-406804183 | |||
| pub fn get_pin_mut(self: PinMut<'a, Self>) -> PinMut<'a, Si> { | |||
| @@ -84,7 +84,8 @@ where | |||
| /// | |||
| /// Note that care must be taken to avoid tampering with the state of the | |||
| /// stream which may otherwise confuse this combinator. | |||
| pub fn get_pin_mut<'a>(self: PinMut<'a, Self>) -> PinMut<'a, St> { | |||
| #[allow(needless_lifetimes)] // https://github.com/rust-lang/rfcs/pull/2362#issuecomment-406804183 | |||
| pub fn get_pin_mut(self: PinMut<'a, Self>) -> PinMut<'a, St> { | |||
|
@Nemo157 It seems that my waker reference optimization PR triggered a clippy warning that broke the CI |
|
Oh, interesting, I didn't realise that Travis tested the merge commit. I was confused how those changes made it into this build. Rebased and fixed the new warning. |
|
@Nemo157 I didn't know that as well. Interesting! |
|
I took a look at the pub fn get_pinned_mut(self: PinMut<'_, Self>) -> PinMut<'_, Si>would work, opened rust-lang/rust#52675 and will update the links in the code now. |
|
@cramertj Are you okay with using in-band lifetimes? (i.e. "may I merge this when it's ready?") |
|
@MajorBreakfast Yup, seems fine to me! I just wanted to check that it was an intentional decision. |
|
I merged this with the intention to remove the |
I think merging all these changes is only worth it if we start gating CI on Clippy, otherwise the buggy allows should be dropped (just means dropping the last 2 commits).
With this and #1111 the deny warnings job could be changed to(EDIT: Done since #1111 is merged)cargo clippy -- -D warningsand hit all builtin + Clippy warnings on all the crates at once.I think the
needless_lifetimesone could be fixed in Clippy relatively easily, presumably it just doesn't supportarbitrary_self_typesproperly. Theredundant_closuresone seems like it won't be able to get a fix soon, it would need Clippy to somehow detect side-effect producing code.I'm opening this mostly for discussion on whether gating CI on Clippy is of interest to the team. If it is then I might take a look at doing the upstream fixes to reduce the bad parts of this.