Re: [PATCH 2/4] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Nick Desaulniers]

On Sat, Oct 5, 2019 at 10:17 AM Miguel Ojeda
<miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>
> Hi Joe,
>
> On Sat, Oct 5, 2019 at 6:46 PM Joe Perches <joe@xxxxxxxxxxx> wrote:
> >
> > Reserve the pseudo keyword 'fallthrough' for the ability to convert the

Have we precedent already for "pseudo keywords?"  I kind of like the
double underscore prefix we use for attributes (which this is one of),
or at least making macro's ALLCAPS as some sort of convention.
Otherwise, someone might be confused on seeing `fallthrough` sprinkled
throughout the code without knowing how it works. `__fallthough` or
`FALLTHROUGH` are maybe less surprising (and potentially easier to
grep)?  Sorry if this has already been discussed; from Miguel's link
below it looks like there used to be underscore prefixes before?

> > various case block /* fallthrough */ style comments to appear to be an
> > actual reserved word with the same gcc case block missing fallthrough
> > warning capability.
> >
> > All switch/case blocks now must end in one of:
> >
> >         break;
> >         fallthrough;
> >         goto <label>;
> >         return [expression];
> >         continue;
> >
> > fallthough is gcc's __attribute__((__fallthrough__)) which was introduced
> > in gcc version 7..
>
> Nits: double period, missing "r" in fallthough.
>
> > fallthrough devolves to an empty "do {} while (0)" if the compiler
> > version (any version less than gcc 7) does not support the attribute.
>
> Perhaps add a short note why (empty statement warnings maybe? I don't
> remember them but it was months ago so maybe it changed).
>
> > Signed-off-by: Joe Perches <joe@xxxxxxxxxxx>
> > Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
>
> Please add Dan's Suggested-by and copy the things I wrote in the
> commit message when I proposed this:
>
>   https://github.com/ojeda/linux/commit/668f011a2706ea555987e263f609a5deba9c7fc4
>
> > ---
> >  include/linux/compiler_attributes.h | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> > index 6b318efd8a74..cdf016596659 100644
> > --- a/include/linux/compiler_attributes.h
> > +++ b/include/linux/compiler_attributes.h
> > @@ -40,6 +40,7 @@
> >  # define __GCC4_has_attribute___noclone__             1
> >  # define __GCC4_has_attribute___nonstring__           0
> >  # define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8)
> > +# define __GCC4_has_attribute___fallthrough__         0
>
> This goes after __externally_visible__.
>
> >  #endif
> >
> >  /*
> > @@ -185,6 +186,22 @@
> >  # define __noclone
> >  #endif
> >
> > +/*
> > + * Add the pseudo keyword 'fallthrough' so case statement blocks
> > + * must end with any of these keywords:
> > + *   break;
> > + *   fallthrough;
> > + *   goto <label>;
> > + *   return [expression];
> > + *
> > + *  gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
>
> This also goes after __externally_visible__.
>
> Please add:
>
>   * Optional: only supported since gcc >= 7.1
>   * Optional: only supported since clang >= 10
>   * Optional: not supported by icc
>
> As well as:
>
>   clang: https://clang.llvm.org/docs/AttributeReference.html#fallthrough
>
> See how I did it in the link above:
>
>   https://github.com/ojeda/linux/commit/668f011a2706ea555987e263f609a5deba9c7fc4
>
> > + */
> > +#if __has_attribute(__fallthrough__)
> > +# define fallthrough                    __attribute__((__fallthrough__))
> > +#else
> > +# define fallthrough                    do {} while (0)  /* fallthrough */
> > +#endif
> > +
> >  /*
> >   * Note the missing underscores.
> >   *
> > --
> > 2.15.0
> >
>
> Cheers,
> Miguel



-- 
Thanks,
~Nick Desaulniers