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

[Alexander Kornienko]

Could anyone, please, look at the patches? Should I commit these? Thanks!

On Tue, May 29, 2012 at 3:08 AM, Alexander Kornienko <alexfh at google.com>wrote:

> 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/cfe-commits/attachments/20120530/9cfbbe41/attachment.html>