[PATCH] mtd: Replace if and BUG with BUG_ON

[PATCH] mtd: Replace if and BUG with BUG_ON

[Amitoj Kaur Chawla]

Replace if condition and BUG() with a BUG_ON having the conditional
expression of the if statement as argument.

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@gmail.com>
---
 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);
 
 	block_address = ssfdc->logic_block_map[block_address];
 
-- 
1.9.1

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@gmail.com> 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@gmail.com>
> ---
>  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

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

[Julia Lawall]

On Mon, 30 May 2016, Ezequiel Garcia wrote:

> Hi Amitoj,
> 
> Thanks for your patch.
> 
> On 28 May 2016 at 13:41, Amitoj Kaur Chawla <amitoj1606@gmail.com> 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@gmail.com>
> > ---
> >  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).

Actually, I think that this particular patch does improve readability a 
bit.  Scanning straight down the code is easier than looking under an if.
Also, git blame now has a way to go back in history (although I don't 
remember what it is), so the argument that cleaning up the code makes it 
very difficult to find why the nontrivial part of the code is as it is 
doesn't completely hold any more.

julia

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

[Brian Norris]

Hi,

On Tue, May 31, 2016 at 07:41:23AM +0200, Julia Lawall wrote:
> On Mon, 30 May 2016, Ezequiel Garcia wrote:
> > On 28 May 2016 at 13:41, Amitoj Kaur Chawla <amitoj1606@gmail.com> wrote:
> > > Replace if condition and BUG() with a BUG_ON having the conditional
> > > expression of the if statement as argument.
[...]

> > > 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).
> 
> Actually, I think that this particular patch does improve readability a 
> bit.  Scanning straight down the code is easier than looking under an if.
> Also, git blame now has a way to go back in history (although I don't 
> remember what it is), so the argument that cleaning up the code makes it 
> very difficult to find why the nontrivial part of the code is as it is 
> doesn't completely hold any more.

I agree it's a small improvement. Not sure I'd worry too much about
git-blame. Applied to l2-mtd.git.

Brian