[2/2] Compiler Attributes: auxdisplay: panel: use __fallthrough

Let gcc know these cases are meant to fall through to the next label
by annotating them with the new __fallthrough statement attribute;
and remove the comment since it conveys the same information
(which was also parsed by gcc to suppress the warning).

Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
 drivers/auxdisplay/panel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

On Sun, 2018-10-21 at 19:14 +0200, Miguel Ojeda wrote:
> Let gcc know these cases are meant to fall through to the next label
> by annotating them with the new __fallthrough statement attribute;
> and remove the comment since it conveys the same information
> (which was also parsed by gcc to suppress the warning).

Instead of many individual conversion patches,
perhaps a script to do all the conversions at once.

Maybe:

pattern='(?:\/\*\s*fall(?:\s*|\s*\-\s*)thr(?:u|ough)\s*\*\/|\/\/\s*fall\s*thr(?:u|ough)\s*$)'
git grep -P -i --name-only "$pattern" -- '*.[ch]' |
    xargs perl -p -i -e "s/$pattern/__fallthrough;/gi"

On Sun, Oct 21, 2018 at 8:11 PM Joe Perches <joe@perches.com> wrote:
>
> On Sun, 2018-10-21 at 19:14 +0200, Miguel Ojeda wrote:
> > Let gcc know these cases are meant to fall through to the next label
> > by annotating them with the new __fallthrough statement attribute;
> > and remove the comment since it conveys the same information
> > (which was also parsed by gcc to suppress the warning).
>
> Instead of many individual conversion patches,
> perhaps a script to do all the conversions at once.

Note that this was only an example of the attribute (some people asked
for an example when introducing another one, so I preemptively did it
for this one).

Doing the conversion (and how :-) I left it for afterwards, if people
agree with the attribute.

>
> Maybe:
>
> pattern='(?:\/\*\s*fall(?:\s*|\s*\-\s*)thr(?:u|ough)\s*\*\/|\/\/\s*fall\s*thr(?:u|ough)\s*$)'
> git grep -P -i --name-only "$pattern" -- '*.[ch]' |
>     xargs perl -p -i -e "s/$pattern/__fallthrough;/gi"


By the way, I checked first if coccinelle could match input comments,
but it doesn't, according to Julia. I am also thinking whether a
compiler plugin could easily do this, but I don't have my hopes high
given these are comments...

Also, regardless of how it is done, the patches need to be sent
individually to maintainers, no? I have a vague memory that big &
automated conversions were a bit frozen upon in the kernel. Greg...?

Cheers,
Miguel

On Sun, Oct 21, 2018 at 10:14 AM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> Let gcc know these cases are meant to fall through to the next label
> by annotating them with the new __fallthrough statement attribute;
> and remove the comment since it conveys the same information
> (which was also parsed by gcc to suppress the warning).
>
> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> ---
>  drivers/auxdisplay/panel.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
> index 21b9b2f2470a..0755034e49ba 100644
> --- a/drivers/auxdisplay/panel.c
> +++ b/drivers/auxdisplay/panel.c
> @@ -1367,7 +1367,7 @@ static void panel_process_inputs(void)
>                                 break;
>                         input->rise_timer = 0;
>                         input->state = INPUT_ST_RISING;
> -                       /* fall through */
> +                       __fallthrough;
>                 case INPUT_ST_RISING:
>                         if ((phys_curr & input->mask) != input->value) {
>                                 input->state = INPUT_ST_LOW;
> @@ -1380,11 +1380,11 @@ static void panel_process_inputs(void)
>                         }
>                         input->high_timer = 0;
>                         input->state = INPUT_ST_HIGH;
> -                       /* fall through */
> +                       __fallthrough;
>                 case INPUT_ST_HIGH:
>                         if (input_state_high(input))
>                                 break;
> -                       /* fall through */
> +                       __fallthrough;
>                 case INPUT_ST_FALLING:
>                         input_state_falling(input);
>                 }
> --
> 2.17.1
>

I would prefer we continue to use the comment style until we've got
confirmed support for (at least) Clang, Coverity, CPPcheck, smatch,
and eclipse.

-Kees

On Mon, 2018-10-22 at 11:51 +0200, Miguel Ojeda wrote:
> On Sun, Oct 21, 2018 at 8:11 PM Joe Perches <joe@perches.com> wrote:
> > On Sun, 2018-10-21 at 19:14 +0200, Miguel Ojeda wrote:
> > > Let gcc know these cases are meant to fall through to the next label
> > > by annotating them with the new __fallthrough statement attribute;
> > > and remove the comment since it conveys the same information
> > > (which was also parsed by gcc to suppress the warning).
> > 
> > Instead of many individual conversion patches,
> > perhaps a script to do all the conversions at once.
> 
> Note that this was only an example of the attribute (some people asked
> for an example when introducing another one, so I preemptively did it
> for this one).
> 
> Doing the conversion (and how :-) I left it for afterwards, if people
> agree with the attribute.
> 
> > Maybe:
> > 
> > pattern='(?:\/\*\s*fall(?:\s*|\s*\-\s*)thr(?:u|ough)\s*\*\/|\/\/\s*fall\s*thr(?:u|ough)\s*$)'
> > git grep -P -i --name-only "$pattern" -- '*.[ch]' |
> >     xargs perl -p -i -e "s/$pattern/__fallthrough;/gi"
> 
> By the way, I checked first if coccinelle could match input comments,
> but it doesn't, according to Julia. I am also thinking whether a
> compiler plugin could easily do this, but I don't have my hopes high
> given these are comments...
> 
> Also, regardless of how it is done, the patches need to be sent
> individually to maintainers, no?

Not really no.  For instance the spdx license treewide change.
commit b24413180f5600bcb3bb70fbed5cf186b60864bd

> I have a vague memory that big &
> automated conversions were a bit frozen upon in the kernel. Greg...?

There really needs to be some method to do treewide
conversions without involving multiple maintainers.

It'd be a good topic for a maintainer summit one day.

Hi Dan,

On Tue, Oct 23, 2018 at 7:37 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Oct 22, 2018 at 07:10:02AM -0700, Kees Cook wrote:
> > I would prefer we continue to use the comment style until we've got
> > confirmed support for (at least) Clang, Coverity, CPPcheck, smatch,
> > and eclipse.
>
> Clang and Smatch don't support fall throught comments.  Coverity
> supports both.  CPPcheck is unknown.
>
> Eclipse doesn't support attributes.  So it's just Eclipse and maybe
> CPP check.

Thanks for checking! Let's wait then a few months and see if we can
get cppcheck/Eclipse to support it.

Cheers,
Miguel

On Fri, Nov 2, 2018 at 11:49 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Thanks for checking! Let's wait then a few months and see if we can
> get cppcheck/Eclipse to support it.

In the meantime, saved here too:

  https://github.com/ojeda/linux/tree/compiler-attributes-fallthrough

rebased on top of e468f5c06b5e ("Merge tag
'compiler-attributes-for-linus-4.20-rc1' of
https://github.com/ojeda/linux").

Cheers,
Miguel