Skip to contentSkip to conversation

Cleanup Clippy issues in drm_panic_qr.rs

PATCH Applied Thomas Böhler 2 participants 9 messages Oct 19, 2024 – Oct 20, 2024

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v2 cover Cleanup Clippy issues in drm_panic_qr.rs

The file drivers/gpu/drm/drm_panic_qr.rs has some lints that Clippy complains about. This series cleans them up by either allowing what is written or conforming to what Clippy expects where it makes sense.

All explicitly allowed lints are marked with #[expect(...)].

v1 -> v2:
- Add "Fixes" trailers and rename "Closes" to "Link" trailers as the patches all fix part of the issue.
- Replace #[allow(...)] with #[expect(...)]. Support for expect is already in rust-next, which is where this series will be merged into.

Thomas Böhler (7):
drm/panic: avoid reimplementing Iterator::find drm/panic: remove unnecessary borrow in alignment_pattern drm/panic: prefer eliding lifetimes
drm/panic: remove redundant field when assigning value drm/panic: correctly indent continuation of line in list item drm/panic: allow verbose boolean for clarity drm/panic: allow verbose version check

 drivers/gpu/drm/drm_panic_qr.rs | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)
Signature · 1 line
2.46.2
#1 lore raw

PATCH v2 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
index 1ef56cb07dfb..76decf49e678 100644
@@ -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 {
Signature · 1 line
2.46.2
#2 lore raw

PATCH v2 2/7 drm/panic: remove unnecessary borrow in alignment_pattern

In reply to Thomas Böhler

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
index 76decf49e678..7adfaa3d6222 100644
@@ -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
#3 lore raw

PATCH v2 3/7 drm/panic: prefer eliding lifetimes

In reply to 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_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
index 7adfaa3d6222..767a8eb0acec 100644
@@ -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: &'a mut [u8]) -> Option<EncodedMsg<'a>> {
+    fn new<'a>(segments: &[&Segment<'_>], data: &'a mut [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
#4 lore raw

PATCH v2 4/7 drm/panic: remove redundant field when assigning value

In reply to 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 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
index 767a8eb0acec..5b2386a515fa 100644
@@ -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
#5 lore raw

PATCH v2 5/7 drm/panic: correctly indent continuation of line in list item

In reply to 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-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
index 5b2386a515fa..58c46f366f76 100644
@@ -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
#6 lore raw

PATCH v2 6/7 drm/panic: allow verbose boolean for clarity

In reply to 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.

Fixes cb5164ac43d0 ("drm/panic: Add a QR code panic screen")

---
v1 -> v2: turn the introduced explicit "return" statement into a block

 drivers/gpu/drm/drm_panic_qr.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_panic_qr.rs +4 -1
index 58c46f366f76..2d4e367f0fcd 100644
@@ -719,7 +719,10 @@ 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)
+        #[expect(clippy::nonminimal_bool)]
+        {
+            (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
#7 lore raw

PATCH v2 7/7 drm/panic: allow verbose version check

In reply to 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)]`

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
index 2d4e367f0fcd..09500cddc009 100644
@@ -983,6 +983,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 {
+    #[expect(clippy::manual_range_contains)]
     if version < 1 || version > 40 {
         return 0;
     }
Signature · 1 line
2.46.2
Applied
#8 lore raw
In reply to Thomas Böhler
Quoted 4 lines · Thomas Böhler

The file drivers/gpu/drm/drm_panic_qr.rs has some lints that Clippy complains about. This series cleans them up by either allowing what is written or conforming to what Clippy expects where it makes sense.

Applied to rust-next -- thanks everyone!

[ Reworded to indent Clippy's message. - Miguel ]

[ Reworded to add Clippy warning like it is done in the rest of the
series. - Miguel ]

[ Reworded to mention the redundant closure cleanup too. - Miguel ]

Cheers,
Miguel