Re: if/else block default coding style question
On Sat, 08 Oct 2016 15:19:18 -, Nicholas Mc Guire said: > Thats actually the cause of this mail - it just becaem obvious > that some of these if==else are intentional At the very least, the intentional ones need a comment similar to the one found in file.c > 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. Coccinelle can only warn about things that syntactically look suspicious. The person running Coccinelle needs to have enough neurons to double-check for comments explaining the odd usage, etc - and the person creating that sort of code needs to leave some bread crumbs for people reading the code. pgpONkz6eRKH1.pgp Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: if/else block default coding style question
On Sat, Oct 08, 2016 at 11:10:10AM -0400, Robert P. J. Day wrote:
> On Sat, 8 Oct 2016, valdis.kletni...@vt.edu wrote:
>
> > On Sat, 08 Oct 2016 10:40:37 -, 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.
>
> if that's the behaviour the developer actually wants, then yes, it's
> messy. but i would be very careful just simplifying it wholesale,
> since it also smacks of a typo where one copy-and-pasted to add the
> default case, then forgot to tweak it to be different.
>
> rather than "fixing" it, i would bring it to the attention of the
> maintainer, and ask him or her to resolve it.
>
sure - no point in fixing code one does not understand.
if at all I send "fixes" out as RFCs if it seems like an obvious
case - and in all other cases a notification/questions are sent
but no patch.
thx!
hofrat
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: if/else block default coding style question
On Sat, Oct 08, 2016 at 10:58:41AM -0400, valdis.kletni...@vt.edu wrote:
> On Sat, 08 Oct 2016 10:40:37 -, 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
* 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);
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
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: if/else block default coding style question
On Sat, 8 Oct 2016, valdis.kletni...@vt.edu wrote:
> On Sat, 08 Oct 2016 10:40:37 -, 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.
if that's the behaviour the developer actually wants, then yes, it's
messy. but i would be very careful just simplifying it wholesale,
since it also smacks of a typo where one copy-and-pasted to add the
default case, then forgot to tweak it to be different.
rather than "fixing" it, i would bring it to the attention of the
maintainer, and ask him or her to resolve it.
rday
--
Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca
Twitter: http://twitter.com/rpjday
LinkedIn: http://ca.linkedin.com/in/rpjday
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: if/else block default coding style question
On Sat, 08 Oct 2016 10:40:37 -, 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.
How did you find there were 90 of them? Did you cook up a Coccinelle script
for it?
If nothing else, cooking up a test to toss into scripts/coccinelle/misc
would probably be a Good Thing...
pgpdPVCaxT45c.pgp
Description: PGP signature
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
