drm/panic: avoid reimplementing Iterator::find
Subsystems (3)
PATCH 1/7 drm/panic: avoid reimplementing Iterator::find
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.
--- drivers/gpu/drm/drm_panic_qr.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/gpu/drm/drm_panic_qr.rs +3 -6
@@ -209,12 +209,9 @@impl Version {/// Returns the smallest QR version than can hold these segments. fn from_segments (segments: &[&Segment <'_>]) ->Option <Version > { -for vin (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 {
Signature · 1 line
2.46.2
PATCH 2/7 drm/panic: remove unnecessary borrow in alignment_pattern
The function alignment_pattern returns a static reference to a u8 slice. The borrow of the returned element in ALIGNMENT_PATTERNS is already a reference as defined in the array definition above so this borrow is unnecessary and removed by the compiler. Clippy notes this in needless_borrow:
error: this expression creates a reference which is immediately dereferenced by the compiler
--> drivers/gpu/drm/drm_panic_qr.rs:245:9
|
245 | &ALIGNMENT_PATTERNS[self.0 - 1]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `ALIGNMENT_PATTERNS[self.0 - 1]`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
= note: `-D clippy::needless-borrow` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::needless_borrow)]`Remove the unnecessary borrow.
--- drivers/gpu/drm/drm_panic_qr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/drm_panic_qr.rs +1 -1
@@ -239,7 +239,7 @@ fn g1_blk_size(&self) -> usize { }fn alignment_pattern (&self) -> &'static [u8 ] { - &ALIGNMENT_PATTERNS [self.0 -1 ] +ALIGNMENT_PATTERNS [self.0 -1 ] }fn poly (&self) -> &'static [u8 ] {
Signature · 1 line
2.46.2
Re: [PATCH 2/7] drm/panic: remove unnecessary borrow in alignment_pattern
Quoted 16 lines · Thomas Böhler
The function
alignment_patternreturns a static reference to au8slice. The borrow of the returned element inALIGNMENT_PATTERNSis already a reference as defined in the array definition above so this borrow is unnecessary and removed by the compiler. Clippy notes this inneedless_borrow:error: this expression creates a reference which is immediately dereferenced by the compiler --> drivers/gpu/drm/drm_panic_qr.rs:245:9 | 245 | &ALIGNMENT_PATTERNS[self.0 - 1] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `ALIGNMENT_PATTERNS[self.0 - 1]` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow = note: `-D clippy::needless-borrow` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::needless_borrow)]`
Quoted 1 line
Remove the unnecessary borrow.
Thanks, it looks good to me.
Quoted 21 lines
Reported-by Miguel Ojeda <[email protected]>Signed-off-by Thomas Böhler <[email protected]>--- drivers/gpu/drm/drm_panic_qr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/drm_panic_qr.rs +1
-1@@ -239,7 +239,7 @@ fn g1_blk_size(&self) -> usize { }fn alignment_pattern (&self) -> &'static [u8 ] { - &ALIGNMENT_PATTERNS [self.0 -1 ] +ALIGNMENT_PATTERNS [self.0 -1 ] }fn poly (&self) -> &'static [u8 ] {
PATCH 3/7 drm/panic: prefer eliding lifetimes
In reply to Thomas BöhlerEliding lifetimes when possible instead of specifying them directly is both shorter and easier to read. Clippy notes this in the needless_lifetimes lint:
error: the following explicit lifetimes could be elided: 'b
--> drivers/gpu/drm/drm_panic_qr.rs:479:16
|
479 | fn new<'a, 'b>(segments: &[&Segment<'b>], data: &'a mut [u8]) -> Option<EncodedMsg<'a>> {
| ^^ ^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
= note: `-D clippy::needless-lifetimes` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::needless_lifetimes)]`
help: elide the lifetimes
|
479 - fn new<'a, 'b>(segments: &[&Segment<'b>], data: &'a mut [u8]) -> Option<EncodedMsg<'a>> {
479 + fn new<'a>(segments: &[&Segment<'_>], data: &'a mut [u8]) -> Option<EncodedMsg<'a>> {
|Remove the explicit lifetime annotation in favour of an elided lifetime.
--- drivers/gpu/drm/drm_panic_qr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/drm_panic_qr.rs +1 -1
@@ -476,7 +476,7 @@ struct EncodedMsg<'a> {/// Data to be put in the QR code, with correct segment encoding, padding, and /// Error Code Correction. impl EncodedMsg <'_> { -fn new <'a, 'b>(segments: &[&Segment <'b>], data: &'amut [u8 ]) ->Option <EncodedMsg <'a>> { +fn new <'a>(segments: &[&Segment <'_>], data: &'amut [u8 ]) ->Option <EncodedMsg <'a>> {let version =Version ::from_segments (segments)?;let ec_size = version.ec_size ();let g1_blocks = version.g1_blocks ();
Signature · 1 line
2.46.2
Re: [PATCH 3/7] drm/panic: prefer eliding lifetimes
Quoted 19 lines · Thomas Böhler
Eliding lifetimes when possible instead of specifying them directly is both shorter and easier to read. Clippy notes this in the
needless_lifetimeslint:error: the following explicit lifetimes could be elided: 'b --> drivers/gpu/drm/drm_panic_qr.rs:479:16 | 479 | fn new<'a, 'b>(segments: &[&Segment<'b>], data: &'a mut [u8]) -> Option<EncodedMsg<'a>> { | ^^ ^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes = note: `-D clippy::needless-lifetimes` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::needless_lifetimes)]` help: elide the lifetimes | 479 - fn new<'a, 'b>(segments: &[&Segment<'b>], data: &'a mut [u8]) -> Option<EncodedMsg<'a>> { 479 + fn new<'a>(segments: &[&Segment<'_>], data: &'a mut [u8]) -> Option<EncodedMsg<'a>> { |
Quoted 2 lines
Remove the explicit lifetime annotation in favour of an elided lifetime.
Thanks, it looks good to me.
Quoted 20 lines
Reported-by Miguel Ojeda <[email protected]>Signed-off-by Thomas Böhler <[email protected]>--- drivers/gpu/drm/drm_panic_qr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/drm_panic_qr.rs +1
-1@@ -476,7 +476,7 @@ struct EncodedMsg<'a> {/// Data to be put in the QR code, with correct segment encoding, padding, and /// Error Code Correction. impl EncodedMsg <'_> { -fn new <'a, 'b>(segments: &[&Segment <'b>], data: &'amut [u8 ]) ->Option <EncodedMsg <'a>> { +fn new <'a>(segments: &[&Segment <'_>], data: &'amut [u8 ]) ->Option <EncodedMsg <'a>> {let version =Version ::from_segments (segments)?;let ec_size = version.ec_size ();let g1_blocks = version.g1_blocks ();
PATCH 4/7 drm/panic: remove redundant field when assigning value
In reply to Thomas BöhlerRust allows initializing fields of a struct without specifying the attribute that is assigned if the variable has the same name. In this instance this is done for all other attributes of the struct except for data.
Remove the redundant data in the assignment to be consistent.
--- drivers/gpu/drm/drm_panic_qr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/drm_panic_qr.rs +1 -1
@@ -489,7 +489,7 @@ fn new<'a>(segments: &[&Segment<'_>], data: &'a mut [u8]) -> Option<EncodedMsg<' data.fill (0 );let mut em =EncodedMsg { - data: data, + data, ec_size, g1_blocks, g2_blocks,
Signature · 1 line
2.46.2
Re: [PATCH 4/7] drm/panic: remove redundant field when assigning value
Quoted 5 lines · Thomas Böhler
Rust allows initializing fields of a struct without specifying the attribute that is assigned if the variable has the same name. In this instance this is done for all other attributes of the struct except for
data.
Remove the redundantdatain the assignment to be consistent.
Thanks, it looks good to me.
Quoted 21 lines
Reported-by Miguel Ojeda <[email protected]>Signed-off-by Thomas Böhler <[email protected]>--- drivers/gpu/drm/drm_panic_qr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/drm_panic_qr.rs +1
-1@@ -489,7 +489,7 @@ fn new<'a>(segments: &[&Segment<'_>], data: &'a mut [u8]) -> Option<EncodedMsg<' data.fill (0 );let mut em =EncodedMsg { - data: data, + data, ec_size, g1_blocks, g2_blocks,
PATCH 5/7 drm/panic: correctly indent continuation of line in list item
In reply to Thomas BöhlerIt is common practice in Rust to indent the next line the same amount of space as the previous one if both belong to the same list item. Clippy checks for this with the lint doc_lazy_continuation.
error: doc list item without indentation
--> drivers/gpu/drm/drm_panic_qr.rs:979:5
|
979 | /// conversion to numeric segments.
| ^
| = help: if this is supposed to be its own paragraph, add a blank line
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_lazy_continuation
= note: -D clippy::doc-lazy-continuation implied by -D warnings
= help: to override -D warnings add #[allow(clippy::doc_lazy_continuation)]
help: indent this line
|
979 | /// conversion to numeric segments.
| ++Indent the offending line by 2 more spaces to remove this Clippy error.
--- drivers/gpu/drm/drm_panic_qr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/drm_panic_qr.rs +1 -1
@@ -976,7 +976,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {/// * `url_len`: Length of the URL. /// /// * If `url_len` > 0, remove the 2 segments header/length and also count the -/// conversion to numeric segments. +/// conversion to numeric segments. /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment. #[no_mangle] pub extern "C" fn drm_panic_qr_max_data_size (version:u8 , url_len:usize ) ->usize {
Signature · 1 line
2.46.2
Re: [PATCH 5/7] drm/panic: correctly indent continuation of line in list item
Quoted 15 lines · Thomas Böhler
It is common practice in Rust to indent the next line the same amount of space as the previous one if both belong to the same list item. Clippy checks for this with the lint
doc_lazy_continuation.error: doc list item without indentation
--> drivers/gpu/drm/drm_panic_qr.rs:979:5 |979 | /// conversion to numeric segments.
| ^
|
= help: if this is supposed to be its own paragraph, add a blank line
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_lazy_continuation
= note:-D clippy::doc-lazy-continuationimplied by-D warnings
= help: to override-D warningsadd#[allow(clippy::doc_lazy_continuation)]
help: indent this line
Quoted 5 lines
| 979 | /// conversion to numeric segments. | ++Indent the offending line by 2 more spaces to remove this Clippy error.
Thanks, it looks good to me.
Quoted 21 lines
Reported-by Miguel Ojeda <[email protected]>Signed-off-by Thomas Böhler <[email protected]>--- drivers/gpu/drm/drm_panic_qr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/drm_panic_qr.rs +1
-1@@ -976,7 +976,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {/// * `url_len`: Length of the URL. /// /// * If `url_len` > 0, remove the 2 segments header/length and also count the -/// conversion to numeric segments. +/// conversion to numeric segments. /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment. #[no_mangle] pub extern "C" fn drm_panic_qr_max_data_size (version:u8 , url_len:usize ) ->usize {
PATCH 6/7 drm/panic: allow verbose boolean for clarity
In reply to Thomas BöhlerClippy complains about a non-minimal boolean expression with nonminimal_bool:
error: this boolean expression can be simplified
--> drivers/gpu/drm/drm_panic_qr.rs:722:9
|
722 | (x < 8 && y < 8) || (x < 8 && y >= end) || (x >= end && y < 8)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
= note: `-D clippy::nonminimal-bool` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::nonminimal_bool)]`
help: try
|
722 | !(x >= 8 || y >= 8 && y < end) || (x >= end && y < 8)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
722 | (y >= end || y < 8) && x < 8 || (x >= end && y < 8)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~While this can be useful in a lot of cases, it isn't here because the line expresses clearly what the intention is. Simplifying the expression means losing clarity, so opt-out of this lint for the offending line.
--- drivers/gpu/drm/drm_panic_qr.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_panic_qr.rs +2 -1
@@ -719,7 +719,8 @@ fn draw_finders(&mut self) {fn is_finder (&self, x:u8 , y:u8 ) ->bool {let end = self.width -8 ; - (x <8 && y <8 ) || (x <8 && y >= end) || (x >= end && y <8 ) +#[allow(clippy::nonminimal_bool)] +return (x <8 && y <8 ) || (x <8 && y >= end) || (x >= end && y <8 ); }// Alignment pattern: 5x5 squares in a grid.
Signature · 1 line
2.46.2
Alice Ryhl
reviewerRe: [PATCH 6/7] drm/panic: allow verbose boolean for clarity
Quoted 36 lines · Thomas Böhler
Clippy complains about a non-minimal boolean expression with
nonminimal_bool:error: this boolean expression can be simplified --> drivers/gpu/drm/drm_panic_qr.rs:722:9 | 722 | (x < 8 && y < 8) || (x < 8 && y >= end) || (x >= end && y < 8) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool = note: `-D clippy::nonminimal-bool` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::nonminimal_bool)]` help: try | 722 | !(x >= 8 || y >= 8 && y < end) || (x >= end && y < 8) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 722 | (y >= end || y < 8) && x < 8 || (x >= end && y < 8) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~While this can be useful in a lot of cases, it isn't here because the line expresses clearly what the intention is. Simplifying the expression means losing clarity, so opt-out of this lint for the offending line.
Reported-by Miguel Ojeda <[email protected]>Signed-off-by Thomas Böhler <[email protected]>--- drivers/gpu/drm/drm_panic_qr.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/gpu/drm/drm_panic_qr.rs +0
-0
drivers/gpu/drm/drm_panic_qr.rs fn draw_finders(&mut self) { · line 723
Quoted 6 lines
drivers/gpu/drm/drm_panic_qr.rs +2
-1@@ -719,7 +719,8 @@ fn draw_finders(&mut self) {fn is_finder (&self, x:u8 , y:u8 ) ->bool {let end = self.width -8 ; - (x <8 && y <8 ) || (x <8 && y >= end) || (x >= end && y <8 ) +#[allow(clippy::nonminimal_bool)] +return (x <8 && y <8 ) || (x <8 && y >= end) || (x >= end && y <8 );
Surely introducing a return statement causes another clippy error?
You can do this:
#[allow(clippy::nonminimal_bool)]
{
(x < 8 && y < 8) || (x < 8 && y >= end) || (x >= end && y < 8)
}or just put the allow on the function.
Alice
Alice Ryhl
reviewerRe: [PATCH 6/7] drm/panic: allow verbose boolean for clarity
In reply to Thomas BöhlerQuoted 36 lines · Thomas Böhler
Clippy complains about a non-minimal boolean expression with
nonminimal_bool:error: this boolean expression can be simplified --> drivers/gpu/drm/drm_panic_qr.rs:722:9 | 722 | (x < 8 && y < 8) || (x < 8 && y >= end) || (x >= end && y < 8) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool = note: `-D clippy::nonminimal-bool` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::nonminimal_bool)]` help: try | 722 | !(x >= 8 || y >= 8 && y < end) || (x >= end && y < 8) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 722 | (y >= end || y < 8) && x < 8 || (x >= end && y < 8) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~While this can be useful in a lot of cases, it isn't here because the line expresses clearly what the intention is. Simplifying the expression means losing clarity, so opt-out of this lint for the offending line.
Reported-by Miguel Ojeda <[email protected]>Signed-off-by Thomas Böhler <[email protected]>--- drivers/gpu/drm/drm_panic_qr.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/gpu/drm/drm_panic_qr.rs +0
-0
drivers/gpu/drm/drm_panic_qr.rs fn draw_finders(&mut self) { · line 723
Quoted 6 lines
drivers/gpu/drm/drm_panic_qr.rs +2
-1@@ -719,7 +719,8 @@ fn draw_finders(&mut self) {fn is_finder (&self, x:u8 , y:u8 ) ->bool {let end = self.width -8 ; - (x <8 && y <8 ) || (x <8 && y >= end) || (x >= end && y <8 ) +#[allow(clippy::nonminimal_bool)] +return (x <8 && y <8 ) || (x <8 && y >= end) || (x >= end && y <8 );
This should be #[expect(...)] instead of #[allow(...)].
Alice
Re: [PATCH 6/7] drm/panic: allow verbose boolean for clarity
In reply to Thomas BöhlerQuoted 19 lines · Thomas Böhler
Clippy complains about a non-minimal boolean expression with
nonminimal_bool:error: this boolean expression can be simplified --> drivers/gpu/drm/drm_panic_qr.rs:722:9 | 722 | (x < 8 && y < 8) || (x < 8 && y >= end) || (x >= end && y < 8) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool = note: `-D clippy::nonminimal-bool` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::nonminimal_bool)]` help: try | 722 | !(x >= 8 || y >= 8 && y < end) || (x >= end && y < 8) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 722 | (y >= end || y < 8) && x < 8 || (x >= end && y < 8) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Quoted 3 lines
While this can be useful in a lot of cases, it isn't here because the line expresses clearly what the intention is. Simplifying the expression means losing clarity, so opt-out of this lint for the offending line.
Thanks, I also prefer to keep the non-minimal boolean.
With the suggestions from Alice Ryhl to not introduce a return, and use expect:
Quoted 22 lines
Reported-by Miguel Ojeda <[email protected]>Signed-off-by Thomas Böhler <[email protected]>--- drivers/gpu/drm/drm_panic_qr.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/gpu/drm/drm_panic_qr.rs +2
-1@@ -719,7 +719,8 @@ fn draw_finders(&mut self) {fn is_finder (&self, x:u8 , y:u8 ) ->bool {let end = self.width -8 ; - (x <8 && y <8 ) || (x <8 && y >= end) || (x >= end && y <8 ) +#[allow(clippy::nonminimal_bool)] +return (x <8 && y <8 ) || (x <8 && y >= end) || (x >= end && y <8 ); }// Alignment pattern: 5x5 squares in a grid.
Re: [PATCH 6/7] drm/panic: allow verbose boolean for clarity
Quoted 3 lines · Jocelyn Falempe
With the suggestions from Alice Ryhl to not introduce a return, and use expect:
+1 to both.
expect (here and the other ones I suggested) require rust-next, so if this goes through DRM, then we can clean this up later. Otherwise, if you prefer rust-next, we can change them to expect already.
Thanks!
Cheers,
Miguel
Re: [PATCH 6/7] drm/panic: allow verbose boolean for clarity
Quoted 10 lines · Miguel Ojeda
Quoted 3 lines · Jocelyn Falempe
With the suggestions from Alice Ryhl to not introduce a return, and use expect:
+1 to both.
expect(here and the other ones I suggested) requirerust-next, so if this goes through DRM, then we can clean this up later. Otherwise, if you preferrust-next, we can change them toexpectalready.
I don't plan to touch drm_panic_qr.rs, so I think it's better if this series goes through rust-next, to avoid an extra cleanup step later.
Signature · 1 line
Jocelyn
Quoted 6 lines
Thanks!
Cheers,
Miguel
PATCH 7/7 drm/panic: allow verbose version check
In reply to Thomas BöhlerClippy warns about a reimplementation of RangeInclusive::contains:
error: manual `!RangeInclusive::contains` implementation
--> drivers/gpu/drm/drm_panic_qr.rs:986:8
|
986 | if version < 1 || version > 40 {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!(1..=40).contains(&version)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
= note: `-D clippy::manual-range-contains` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::manual_range_contains)]`Ignore this and keep the current implementation as that makes it easier to read.
--- drivers/gpu/drm/drm_panic_qr.rs | 1 + 1 file changed, 1 insertion(+)
drivers/gpu/drm/drm_panic_qr.rs +1 -0
@@ -981,6 +981,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {/// * If `url_len` = 0, only removes 3 bytes for 1 binary segment. #[no_mangle] pub extern "C" fn drm_panic_qr_max_data_size (version:u8 , url_len:usize ) ->usize { +#[allow(clippy::manual_range_contains)] if version <1 || version >40 {return 0 ; }
Signature · 1 line
2.46.2
Re: [PATCH 7/7] drm/panic: allow verbose version check
Quoted 13 lines · Thomas Böhler
Clippy warns about a reimplementation of
RangeInclusive::contains:error: manual `!RangeInclusive::contains` implementation --> drivers/gpu/drm/drm_panic_qr.rs:986:8 | 986 | if version < 1 || version > 40 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!(1..=40).contains(&version)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains = note: `-D clippy::manual-range-contains` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_range_contains)]`
Quoted 2 lines
Ignore this and keep the current implementation as that makes it easier to read.
Yeah, I wonder if we may want to disable globally this one (and possibly the previous one too) -- I am ambivalent.
Quoted 1 line
+ #[allow(clippy::manual_range_contains)]
This (and the previous one) may be good candidates for #[expect]. We don't have that yet in mainline, but it is in rust-next, so we can clean it up next cycle.
Cheers,
Miguel
Re: [PATCH 7/7] drm/panic: allow verbose version check
In reply to Thomas BöhlerQuoted 12 lines · Thomas Böhler
Clippy warns about a reimplementation of
RangeInclusive::contains:error: manual `!RangeInclusive::contains` implementation --> drivers/gpu/drm/drm_panic_qr.rs:986:8 | 986 | if version < 1 || version > 40 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!(1..=40).contains(&version)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains = note: `-D clippy::manual-range-contains` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_range_contains)]`
Quoted 2 lines
Ignore this and keep the current implementation as that makes it easier to read.
Thanks, it looks good to me.
Quoted 20 lines
Reported-by Miguel Ojeda <[email protected]>Signed-off-by Thomas Böhler <[email protected]>--- drivers/gpu/drm/drm_panic_qr.rs | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/drm_panic_qr.rs +1
-0@@ -981,6 +981,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {/// * If `url_len` = 0, only removes 3 bytes for 1 binary segment. #[no_mangle] pub extern "C" fn drm_panic_qr_max_data_size (version:u8 , url_len:usize ) ->usize { +#[allow(clippy::manual_range_contains)] if version <1 || version >40 {return 0 ; }
Hi Thomas,
These commit logs are nicely explained -- thanks a lot for taking the time to write each!
A couple nits below.
Quoted 3 lines · Thomas Böhler
implementing the same logic itself.
Clippy complains about this in themanual_findlint:
Typically commit messages use newlines between paragraphs.
Quoted 2 lines
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:
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.
Thanks again!
Cheers,
Miguel
Quoted 22 lines · Miguel Ojeda
Hi Thomas,
These commit logs are nicely explained -- thanks a lot for taking the time to write each!
A couple nits below.
Quoted 3 lines · Thomas Böhler
implementing the same logic itself.
Clippy complains about this in themanual_findlint:Typically commit messages use newlines between paragraphs.
Quoted 2 lines
Since each of these commits fixes part of the issue, I think these are meant to be
Link:s instead ofCloses:s according to the docs:
Quoted 9 lines
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
Quoted 5 lines
Thanks again!
Cheers,
Miguel
Quoted 1 line · Jocelyn Falempe
Hi Thomas,
Hi Jocelyn,
Quoted 5 lines
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.
Quoted 5 lines
Best regards,
--
Jocelyn
Kind regards,
Signature · 2 lines
Thomas Böhler https://wiredspace.de
Quoted 1 line · Miguel Ojeda
Hi Thomas,
Hi Miguel,
Quoted 7 lines · Thomas Böhler
Quoted 3 lines
implementing the same logic itself.
Clippy complains about this in themanual_findlint:Typically commit messages use newlines between paragraphs.
I wanted to logically group these sentences together, but can also use paragraphs of course.
Quoted 10 lines
Quoted 2 lines
Since each of these commits fixes part of the issue, I think these are meant to be
Link:s instead ofCloses:s according to the docs: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.
Quoted 5 lines
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 :)
Quoted 1 line
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.
Quoted 2 lines
Cheers,
Miguel
Kind regards,
Signature · 2 lines
Thomas Böhler https://wiredspace.de
Quoted 21 lines · Thomas Böhler
Rust's standard library's
std::iter::Iteratortrait provides a functionfindthat finds the first element that satisfies a predicate. The functionVersion::from_segmentsis doing the same thing but is implementing the same logic itself.
Clippy complains about this in themanual_findlint: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)]`
Quoted 1 line
Use
Iterator::findinstead to make the intention clearer.
Thanks for this patch, and the whole series.
It's a nice cleanup.
Signature · 1 line
Jocelyn
Quoted 28 lines
Reported-by Miguel Ojeda <[email protected]>Signed-off-by Thomas Böhler <[email protected]>--- drivers/gpu/drm/drm_panic_qr.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)drivers/gpu/drm/drm_panic_qr.rs +3
-6@@ -209,12 +209,9 @@impl Version {/// Returns the smallest QR version than can hold these segments. fn from_segments (segments: &[&Segment <'_>]) ->Option <Version > { -for vin (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 {