Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

[Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>]

Hi Dan,

On Sat, Oct 20, 2018 at 9:22 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sat, Oct 20, 2018 at 04:42:07PM +0200, Miguel Ojeda wrote:
> > Using an attribute is indeed better whenever possible. In C++17 it is
> > an standard attribute and there have been proposals to include some of
> > them for C as well since 2016 at least, e.g. the latest for
> > fallthrough at:
> >
> >   http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf
> >
> > I have taken a quick look into supporting it (typing it here to save
> > it on the mailing list :-), and we have:
> >
> >   * gcc >= 7.1 supports -Wimplicit-fallthrough with
> > __attribute__((fallthrough)), the comment syntax and the C++
> > [[fallthrough]] syntax.
> >   * gcc < 7.1 complains about empty declarations (it does not know
> > about attributes for null statements) and also
> > -Wdeclaration-after-statement.
>
> I'm not sure I understand about empty decalarations.  The idea is that
> we would do:

That paragraph tried to explain that gcc < 7.1 did not know about
__attribute__((fallthrough)); i.e. that everything was introduced in
gcc 7.1.

Anyway, the conclusion was a neuron misfiring of mine -- in my mind I
was thinking clang supported the comment syntax (when I clearly wrote
that it didn't). Never mind --- thanks for pointing it out!

>
>         case 3:
>                 frob();
>                 __fall_through();
>         case 4:
>                 frob();
>
> #if GCC > 7.1
> #define __fall_through() __attribute__((fallthrough))
> #else
> #define __fall_through()
> #endif

Yes, of course, that is what we do for other attributes -- actually in
-next we have pending a better way for checking, using
__has_attribute:

  #if __has_attribute(fallthrough)
  #define __fallthrough __attribute__((fallthrough))
  #else
  #define __fallthrough
  #endif

>
> So long as GCC and static analysis tools understand about the attribute
> that's enought to catch the bugs. No one cares what clang and icc do.

Not so sure about that -- there are quite some people looking forward
to building Linux with clang, even if only to have another compiler to
check for warnings and to use the clang/llvm-related tools (and to
write new ones).

> We would just disable the fall through warnings on those until they are
> updated to support the fallthrough attribute.

You mean if they start supporting the warning but not the attribute? I
don't think that would be likely (i.e. if clang enables the warning on
C mode, they will have to introduce a way to suppress it; which should
be the attribute (at least), since they typically like to be
compatible with gcc and since they already support it in C++ >= 11),
but everything is possible.

>
> We wouldn't update all the fall through comments until later, but going
> forward people could use the __fall_through() macro if they want.

Agreed. I will introduce it in the compiler-attributes tree -- should
be there for -rc1 if no one complains. CC'ing all of you in the patch.

Cheers,
Miguel