| Message ID | 20241012075312.16342-1-witcher@wiredspace.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] drm/panic: avoid reimplementing Iterator::find | expand |
On 12/10/2024 09:52, Thomas Böhler wrote: > Rust's standard library's `std::iter::Iterator` trait provides a function > `find` that finds the first element that satisfies a predicate. > The function `Version::from_segments` is doing the same thing but is > implementing the same logic itself. > Clippy complains about this in the `manual_find` lint: > > error: manual implementation of `Iterator::find` > --> drivers/gpu/drm/drm_panic_qr.rs:212:9 > | > 212 | / for v in (1..=40).map(|k| Version(k)) { > 213 | | if v.max_data() * 8 >= segments.iter().map(|s| s.total_size_bits(v)).sum() { > 214 | | return Some(v); > 215 | | } > 216 | | } > 217 | | None > | |____________^ help: replace with an iterator: `(1..=40).map(|k| Version(k)).find(|&v| v.max_data() * 8 >= segments.iter().map(|s| s.total_size_bits(v)).sum())` > | > = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_find > = note: `-D clippy::manual-find` implied by `-D warnings` > = help: to override `-D warnings` add `#[allow(clippy::manual_find)]` > > Use `Iterator::find` instead to make the intention clearer. Thanks for this patch, and the whole series. It's a nice cleanup. Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
On 12/10/2024 13:04, Miguel Ojeda wrote: > Hi Thomas, > > These commit logs are nicely explained -- thanks a lot for taking the > time to write each! > > A couple nits below. > > On Sat, Oct 12, 2024 at 9:53 AM Thomas Böhler <witcher@wiredspace.de> wrote: >> >> implementing the same logic itself. >> Clippy complains about this in the `manual_find` lint: > > Typically commit messages use newlines between paragraphs. > >> Reported-by: Miguel Ojeda <ojeda@kernel.org> >> Closes: https://github.com/Rust-for-Linux/linux/issues/1123 > > Since each of these commits fixes part of the issue, I think these are > meant to be `Link:`s instead of `Closes:`s according to the docs: > > https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > > In addition, these should probably have a `Fixes:` tag too -- I should > have mentioned that in the issue, sorry. > > Finally, as a suggestion for the future: for a series like this, it > may make sense to have a small/quick cover letter saying something as > simple as: "Clippy reports some issues in ... -- this series cleans > them up.". Having a cover letter also allows you to give a title to > the series. > Hi Thomas, If you want to send a v2, the easiest way is to download the mbox series from https://patchwork.freedesktop.org/series/139924/ and apply it with git am. That way you will have my reviewed-by automatically added. I can push this series through drm-misc-next if needed. Best regards, -- Jocelyn > Thanks again! > > Cheers, > Miguel >
On Sat Oct 12, 2024 at 1:04 PM CEST, Miguel Ojeda wrote: > Hi Thomas, Hi Miguel, > > On Sat, Oct 12, 2024 at 9:53 AM Thomas Böhler <witcher@wiredspace.de> wrote: > > > > implementing the same logic itself. > > Clippy complains about this in the `manual_find` lint: > > Typically commit messages use newlines between paragraphs. I wanted to logically group these sentences together, but can also use paragraphs of course. > > Reported-by: Miguel Ojeda <ojeda@kernel.org> > > Closes: https://github.com/Rust-for-Linux/linux/issues/1123 > > Since each of these commits fixes part of the issue, I think these are > meant to be `Link:`s instead of `Closes:`s according to the docs: > > https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > > In addition, these should probably have a `Fixes:` tag too -- I should > have mentioned that in the issue, sorry. Good point, I didn't realise this when I read the documentation. I'll change/add the trailer as suggested. > Finally, as a suggestion for the future: for a series like this, it > may make sense to have a small/quick cover letter saying something as > simple as: "Clippy reports some issues in ... -- this series cleans > them up.". Having a cover letter also allows you to give a title to > the series. Makes sense, v2 will have a cover letter :) > Thanks again! Thank you for the nits, they're exactly what I've been looking forward to! I'll prepare a v2 within the coming days as I'm currently limited on free time, so thank you in advance for the patience. > Cheers, > Miguel Kind regards,
On Mon Oct 14, 2024 at 11:06 AM CEST, Jocelyn Falempe wrote: > Hi Thomas, Hi Jocelyn, > If you want to send a v2, the easiest way is to download the mbox series > from https://patchwork.freedesktop.org/series/139924/ > and apply it with git am. > > That way you will have my reviewed-by automatically added. That's neat to know, thank you! That makes the use-case of patchwork a bit clearer for me. > Best regards, > > -- > > Jocelyn Kind regards,
diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs index 1ef56cb07dfb..76decf49e678 100644 --- a/drivers/gpu/drm/drm_panic_qr.rs +++ b/drivers/gpu/drm/drm_panic_qr.rs @@ -209,12 +209,9 @@ impl Version { /// Returns the smallest QR version than can hold these segments. fn from_segments(segments: &[&Segment<'_>]) -> Option<Version> { - for v in (1..=40).map(|k| Version(k)) { - if v.max_data() * 8 >= segments.iter().map(|s| s.total_size_bits(v)).sum() { - return Some(v); - } - } - None + (1..=40) + .map(Version) + .find(|&v| v.max_data() * 8 >= segments.iter().map(|s| s.total_size_bits(v)).sum()) } fn width(&self) -> u8 {
Rust's standard library's `std::iter::Iterator` trait provides a function `find` that finds the first element that satisfies a predicate. The function `Version::from_segments` is doing the same thing but is implementing the same logic itself. Clippy complains about this in the `manual_find` lint: error: manual implementation of `Iterator::find` --> drivers/gpu/drm/drm_panic_qr.rs:212:9 | 212 | / for v in (1..=40).map(|k| Version(k)) { 213 | | if v.max_data() * 8 >= segments.iter().map(|s| s.total_size_bits(v)).sum() { 214 | | return Some(v); 215 | | } 216 | | } 217 | | None | |____________^ help: replace with an iterator: `(1..=40).map(|k| Version(k)).find(|&v| v.max_data() * 8 >= segments.iter().map(|s| s.total_size_bits(v)).sum())` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_find = note: `-D clippy::manual-find` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_find)]` Use `Iterator::find` instead to make the intention clearer. Reported-by: Miguel Ojeda <ojeda@kernel.org> Closes: https://github.com/Rust-for-Linux/linux/issues/1123 Signed-off-by: Thomas Böhler <witcher@wiredspace.de> --- drivers/gpu/drm/drm_panic_qr.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)