Messages in this thread | | | From | Miguel Ojeda <> | Date | Sun, 21 Oct 2018 06:15:53 +0200 | Subject | Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up |
| |
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
| |