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

[Alexander Kornienko]

Any thoughts on these patches/proposals?

On Mon, Jun 4, 2012 at 9:24 PM, Alexander Kornienko <alexfh at google.com>wrote:

> Hi all,
>
> I'm reposting this message, as it seems to be lost. It would be really
> nice if someone could take a look at llvm changes and find out if they
> relate to real bugs or not. I would also appreciate to hear opinions on the
> suggested LLVM_FALLTHROUGH macro as a compiler-verifiable way to annotate
> fall-throughs in switch statements.
>
> Thank you!
>
> 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 LLVM_FALLTHROUGH [[clang::fallthrough]]
>> **#endif
>> **#endif
>> ** **#ifndef LLVM_FALLTHROUGH
>> **#define LLVM_FALLTHROUGH do { } while (0)
>> **#endif*
>
>
>
> --
> Regards,
> Alexander Kornienko
>



-- 
Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151 221
77 957
Google Germany GmbH | Dienerstr. 12 | 80331 München
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120611/8b640963/attachment.html>
-------------- 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/20120611/8b640963/attachment.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/20120611/8b640963/attachment-0001.obj>