[PATCH 1/7] drm/panic: avoid reimplementing Iterator::find

[PATCH 1/7] drm/panic: avoid reimplementing Iterator::find

[Thomas Böhler]

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(-)

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 {
-- 
2.46.2

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

[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.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
index 76decf49e678..7adfaa3d6222 100644
--- a/drivers/gpu/drm/drm_panic_qr.rs
+++ b/drivers/gpu/drm/drm_panic_qr.rs
@@ -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] {
-- 
2.46.2

Re: [PATCH 2/7] drm/panic: remove unnecessary borrow in alignment_pattern

[Jocelyn Falempe]

On 12/10/2024 09:52, Thomas Böhler wrote:
> 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.

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>


> 
> 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 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> index 76decf49e678..7adfaa3d6222 100644
> --- a/drivers/gpu/drm/drm_panic_qr.rs
> +++ b/drivers/gpu/drm/drm_panic_qr.rs
> @@ -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

[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.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
index 7adfaa3d6222..767a8eb0acec 100644
--- a/drivers/gpu/drm/drm_panic_qr.rs
+++ b/drivers/gpu/drm/drm_panic_qr.rs
@@ -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();
-- 
2.46.2

Re: [PATCH 3/7] drm/panic: prefer eliding lifetimes

[Jocelyn Falempe]

On 12/10/2024 09:52, Thomas Böhler wrote:
> 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.
> 

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> 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 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> index 7adfaa3d6222..767a8eb0acec 100644
> --- a/drivers/gpu/drm/drm_panic_qr.rs
> +++ b/drivers/gpu/drm/drm_panic_qr.rs
> @@ -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();

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

[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.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
index 767a8eb0acec..5b2386a515fa 100644
--- a/drivers/gpu/drm/drm_panic_qr.rs
+++ b/drivers/gpu/drm/drm_panic_qr.rs
@@ -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,
-- 
2.46.2

Re: [PATCH 4/7] drm/panic: remove redundant field when assigning value

[Jocelyn Falempe]

On 12/10/2024 09:52, Thomas Böhler wrote:
> 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.

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> 
> 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 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> index 767a8eb0acec..5b2386a515fa 100644
> --- a/drivers/gpu/drm/drm_panic_qr.rs
> +++ b/drivers/gpu/drm/drm_panic_qr.rs
> @@ -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

[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.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
index 5b2386a515fa..58c46f366f76 100644
--- a/drivers/gpu/drm/drm_panic_qr.rs
+++ b/drivers/gpu/drm/drm_panic_qr.rs
@@ -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 {
-- 
2.46.2

Re: [PATCH 5/7] drm/panic: correctly indent continuation of line in list item

[Jocelyn Falempe]

On 12/10/2024 09:52, Thomas Böhler wrote:
> 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.

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> 
> 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 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> index 5b2386a515fa..58c46f366f76 100644
> --- a/drivers/gpu/drm/drm_panic_qr.rs
> +++ b/drivers/gpu/drm/drm_panic_qr.rs
> @@ -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

[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 <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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
index 58c46f366f76..226107c02679 100644
--- a/drivers/gpu/drm/drm_panic_qr.rs
+++ b/drivers/gpu/drm/drm_panic_qr.rs
@@ -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.
-- 
2.46.2

Re: [PATCH 6/7] drm/panic: allow verbose boolean for clarity

[Alice Ryhl]

On Sat, Oct 12, 2024 at 9:53 AM Thomas Böhler <witcher@wiredspace.de> wrote:
>
> 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 <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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> index 58c46f366f76..226107c02679 100644
> --- a/drivers/gpu/drm/drm_panic_qr.rs
> +++ b/drivers/gpu/drm/drm_panic_qr.rs
> @@ -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

Re: [PATCH 6/7] drm/panic: allow verbose boolean for clarity

[Alice Ryhl]

On Sat, Oct 12, 2024 at 9:53 AM Thomas Böhler <witcher@wiredspace.de> wrote:
>
> 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 <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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> index 58c46f366f76..226107c02679 100644
> --- a/drivers/gpu/drm/drm_panic_qr.rs
> +++ b/drivers/gpu/drm/drm_panic_qr.rs
> @@ -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

[Jocelyn Falempe]

On 12/10/2024 09:52, Thomas Böhler wrote:
> 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.

Thanks, I also prefer to keep the non-minimal boolean.

With the suggestions from Alice Ryhl to not introduce a return, and use 
expect:

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> 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 | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> index 58c46f366f76..226107c02679 100644
> --- a/drivers/gpu/drm/drm_panic_qr.rs
> +++ b/drivers/gpu/drm/drm_panic_qr.rs
> @@ -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

[Miguel Ojeda]

On Mon, Oct 14, 2024 at 10:54 AM Jocelyn Falempe <jfalempe@redhat.com> wrote:
>
> 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

[Jocelyn Falempe]

On 14/10/2024 18:59, Miguel Ojeda wrote:
> On Mon, Oct 14, 2024 at 10:54 AM Jocelyn Falempe <jfalempe@redhat.com> wrote:
>>
>> 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.

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.

-- 

Jocelyn

> 
> Thanks!
> 
> Cheers,
> Miguel
> 

[PATCH 7/7] drm/panic: allow verbose version check

[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.

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
index 226107c02679..fe842466d8d6 100644
--- a/drivers/gpu/drm/drm_panic_qr.rs
+++ b/drivers/gpu/drm/drm_panic_qr.rs
@@ -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;
     }
-- 
2.46.2

Re: [PATCH 7/7] drm/panic: allow verbose version check

[Miguel Ojeda]

On Sat, Oct 12, 2024 at 9:54 AM Thomas Böhler <witcher@wiredspace.de> wrote:
>
> 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.

Yeah, I wonder if we may want to disable globally this one (and
possibly the previous one too) -- I am ambivalent.

> +    #[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

[Jocelyn Falempe]

On 12/10/2024 09:52, Thomas Böhler wrote:
> 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.

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> 
> 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 | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> index 226107c02679..fe842466d8d6 100644
> --- a/drivers/gpu/drm/drm_panic_qr.rs
> +++ b/drivers/gpu/drm/drm_panic_qr.rs
> @@ -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;
>       }

Re: [PATCH 1/7] drm/panic: avoid reimplementing Iterator::find

[Miguel Ojeda]

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.

Thanks again!

Cheers,
Miguel

Re: [PATCH 1/7] drm/panic: avoid reimplementing Iterator::find

[Jocelyn Falempe]

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
> 

Re: [PATCH 1/7] drm/panic: avoid reimplementing Iterator::find

[Thomas Böhler]

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,

-- 
Thomas Böhler
https://wiredspace.de

Re: [PATCH 1/7] drm/panic: avoid reimplementing Iterator::find

[Thomas Böhler]

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,

-- 
Thomas Böhler
https://wiredspace.de

Re: [PATCH 1/7] drm/panic: avoid reimplementing Iterator::find

[Jocelyn Falempe]

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>

-- 

Jocelyn


> 
> 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(-)
> 
> 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 {