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

[Alexander Kornienko]

Hi llvm-commits,

Did anyone have a chance to look at these patches?

On Wed, May 30, 2012 at 2:28 PM, Alexander Kornienko <alexfh at google.com>wrote:

> 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
>
>
>


-- 
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/20120602/b5d355f6/attachment.html>