Re: [PATCH] mtd: Replace if and BUG with BUG_ON

[Ezequiel Garcia]

Hi Amitoj,

Thanks for your patch.

On 28 May 2016 at 13:41, Amitoj Kaur Chawla <amitoj1606@xxxxxxxxx> wrote:
> Replace if condition and BUG() with a BUG_ON having the conditional
> expression of the if statement as argument.
>

We usually want commit messages that tell us *why* you are doing the
change: what are you fixing, or what are you improving, and what
possible side-effects it may have.

This commit log explains what the code does, but we can clearly see
that, so it's not useful.

> The Coccinelle semantic patch used to make this change is as follows:
> @@ expression E,f; @@
>
> (
>   if (<+... f(...) ...+>) { BUG(); }
> |
> - if (E) { BUG(); }
> + BUG_ON(E);
> )
>
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@xxxxxxxxx>
> ---
>  drivers/mtd/ssfdc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/ssfdc.c b/drivers/mtd/ssfdc.c
> index daf82ba..41b13d1 100644
> --- a/drivers/mtd/ssfdc.c
> +++ b/drivers/mtd/ssfdc.c
> @@ -380,8 +380,7 @@ static int ssfdcr_readsect(struct mtd_blktrans_dev *dev,
>                 " block_addr=%d\n", logic_sect_no, sectors_per_block, offset,
>                 block_address);
>
> -       if (block_address >= ssfdc->map_len)
> -               BUG();
> +       BUG_ON(block_address >= ssfdc->map_len);
>

I don't want to be rude, but I wonder if there's any value at all in
such a patch. It barely improves readability, it barely reduces the
LoC, yet it consumes developer time, maintainer time, and changes git
per-line authorship (used in git blame).

I'm not complaining about *this* particular patch, but rather about
these kind of supposedly clean-up patches.
-- 
Ezequiel GarcÃa, VanguardiaSur
www.vanguardiasur.com.ar