[llvm-commits] [cfe-commits] [PATCH] Fixes for unintended fall-through bugs (-Wimplicit-fallthrough)

[Alexander Kornienko]

Hi,

I've put aside header changes for now. There are patches with bug fixes (at
least, those seem to be bugs to me) and a patch with LLVM_FALLTHROUGH macro.

Please, take a look at these. Thanks!

On Sun, May 27, 2012 at 5:01 AM, Jordy Rose <jediknil at belkadan.com> wrote:

> I like the idea of adopting a fallthrough notation in our sources. I would
> suggest naming it LLVM_FALLTHROUGH, though, in line with most of the rest
> of llvm/Support/Compiler.h.
>
> The 6 bugs you found seem scary, though I guess they can't be awful or
> we'd have found them sooner. I'm not happy with the header changes, though
> -- the asserts are both a lot hairier this way.
>
> Jordy
>
>
> On May 26, 2012, at 15:30, Alexander Kornienko wrote:
>
> > Hello llvm-commits, clang-commits,
> >
> > I compiled llvm and clang with the recently added -Wimplicit-fallthrough
> diagnostic and analyzed the results. For those who are curious: there are
> about 400 'unannotated fall-through' warnings in total, about 290
> fall-through locations are annotated in comments, about 30 locations
> fall-through to an empty block (case 1: ....<no break>  case 2: break;), 7
> locations have assert(false) not followed by break;
> >
> > From the remaining ~70 locations 6 look like real bugs. I've prepared
> two patches: for llvm and clang, which add break; for all these locations.
> I've also removed two unnecessary fall-throughs in headers to reduce total
> amount of 'unannotated fall-through' warning messages.
> >
> > I can't guarantee that all these 6 locations are real bugs, but they
> look very much like unintended fall-throughs.
> >
> > Could you, please, review this patch?
> > Thanks!
> >
> > P.S. It would also be interesting to know what people think about
> converting fall-through annotations in comments to a checked annotation so
> we could use this diagnostic on regular basis?
> > We could wrap [[clang::fallthrough]] to a macro, so the code would
> continue to work in c++98 mode, but in c++11 mode it would also allow to
> run this diagnostic (btw, is llvm compilable in c++11 mode?). Sample
> implementation of macro:
> > #ifdef __clang__
> > #if __has_feature(cxx_attributes) &&
> __has_warning("-Wimplicit-fallthrough")
> > #define FALLTHROUGH [[clang::fallthrough]]
> > #endif
> > #endif
> >
> > #ifndef FALLTHROUGH
> > #define FALLTHROUGH do { } while (0)
> > #endif
> >
> >
> > --
> > Best regards,
> > Alexander Kornienko
>

-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120529/0c3047c6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fallthrough-bugs-clang.diff
Type: application/octet-stream
Size: 1113 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120529/0c3047c6/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fallthrough-bugs-llvm.diff
Type: application/octet-stream
Size: 2402 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120529/0c3047c6/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fallthrough-macro.diff
Type: application/octet-stream
Size: 1798 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120529/0c3047c6/attachment-0002.obj>