Re: [PATCH v2 5/5] mtd: spi-nor: swp: Drop 'else' after 'return'

[<Tudor.Ambarus@microchip.com>]

On 3/15/21 8:53 AM, Joe Perches wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, 2021-03-08 at 11:58 +0530, Pratyush Yadav wrote:
>> On 06/03/21 11:50AM, Tudor Ambarus wrote:
>>> else is not generally useful after a break or return.
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>
> 
> I don't think this improves the code.
> 
> Generally, checkpatch is a stupid little script.
> 
> This code uses a form like:
>         if (foo)
>                 return bar;
>         else
>                 return baz;

Isn't else redundant? What are the benefits of keeping the else?

> 
> which checkpatch recognizes as OK and so checkpatch does not
> emit any warning message, but this code just adds comments
> before each return which confuses checkpatch.
> 
> I think better would be to change the code to use temporaries
> and convert the functions to bool.
> 
> Something like:
> ---
>  drivers/mtd/spi-nor/core.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0522304f52fa..e174a2f1d621 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1798,36 +1798,41 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
>  }
> 
>  /*
> - * Return 1 if the entire region is locked (if @locked is true) or unlocked (if
> - * @locked is false); 0 otherwise
> + * Return true if the entire region is locked
> + * (if @locked is true) or unlocked (if @locked is false); false otherwise
>   */
> -static int spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
> +static bool spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
>                                         uint64_t len, u8 sr, bool locked)
>  {
>         loff_t lock_offs;
>         uint64_t lock_len;
> +       uint64_t lock_max;
> +       uint64_t ofs_max;
> 
>         if (!len)
> -               return 1;
> +               return true;

returning one is wrong indeed, would you submit a patch for the conversion
of the functions to bool?

> 
>         spi_nor_get_locked_range_sr(nor, sr, &lock_offs, &lock_len);
> 
> +       lock_max = lock_offs + lock_len;
> +       ofs_max = ofs + len;
> +
>         if (locked)
>                 /* Requested range is a sub-range of locked range */
> -               return (ofs + len <= lock_offs + lock_len) && (ofs >= lock_offs);
> +               return (ofs_max <= lock_max) && (ofs >= lock_offs);
>         else
>                 /* Requested range does not overlap with locked range */
> -               return (ofs >= lock_offs + lock_len) || (ofs + len <= lock_offs);
> +               return (ofs >= lock_max) || (ofs_max <= lock_offs);

This should be fine too.

Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/