From mboxrd@z Thu Jan  1 00:00:00 1970
From: der.herr@hofr.at (Nicholas Mc Guire)
Date: Sat, 8 Oct 2016 15:19:18 +0000
Subject: if/else block default coding style question
In-Reply-To: <78160.1475938721@turing-police.cc.vt.edu>
References: <20161008104037.GA493@osadl.at>
	<78160.1475938721@turing-police.cc.vt.edu>
Message-ID: <20161008151918.GA1716@osadl.at>
To: kernelnewbies@lists.kernelnewbies.org
List-Id: kernelnewbies.lists.kernelnewbies.org

On Sat, Oct 08, 2016 at 10:58:41AM -0400, Valdis.Kletnieks at vt.edu wrote:
> On Sat, 08 Oct 2016 10:40:37 -0000, Nicholas Mc Guire said:
> 
> >    } else if (rtlpcipriv->bt_coexist.bt_service == BT_PAN) {
> >            rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
> >    } else {
> >            rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
> >    }
> 
> That *does* smell like a bug.  If nothing else, the last 'else if'
> can be removed.  Most likely case: somebody cut-n-pasted that last
> section in and failed to change it to a proper 'default' value
> and the code falls through to that one rarely enough that nobody has
> noticed.
> 
> >  I personally find this irritating as (without a comment) it is hard to say
> >  if this is a trivial type -> missed case, or if this is
> >  intended as a default behavior.
> 
> Unfortunately, it will probably be a really rough job figuring out what
> exactly was intended in each case.  Figuring it out in filesystem code
> or other similar areas of the kernel wouldn't be too bad - but if it's
> a hardware driver, you're going to have to ask an expert (or somebody who
> has the hardware) for help.

Actually fs had one legitimate case where the positional side-effect was in use

<snip: fs/kernfs/file.c:kernfs_fop_open()>
       * Both paths of the branch look the same.  They're supposed to
       * look that way and give @of->mutex different static lockdep keys.
       */
      if (has_mmap)
              mutex_init(&of->mutex);
      else
              mutex_init(&of->mutex);
<snip>

but almost all of the other cases look phony

> 
> How did you find there were 90 of them?  Did you cook up a Coccinelle script
> for it?
>

simple cocci script - Im checking through the reported cases to
see if it is actually reliable. In some cases it was obvious bugs (cut&paste
type) but in others it does seem to be a "coding pattern".
 
> If nothing else, cooking up a test to toss into scripts/coccinelle/misc
> would probably be a Good Thing...

Thats actually the cause of this mail - it just becaem obvious
that some of these if==else are intentional

Now reporting these cases if they should be fixed makes sense
while if this is accepted practice then it would be reporting
false-positives, which would be bad.

thx!
hofrat