Could anyone, please, look at the patches? Should I commit these? Thanks!<br><br><div class="gmail_quote">On Tue, May 29, 2012 at 3:08 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<div><br></div><div>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.</div>
<div><br></div><div>Please, take a look at these. Thanks!<div><div class="h5"><br>
<br><div class="gmail_quote">On Sun, May 27, 2012 at 5:01 AM, Jordy Rose <span dir="ltr"><<a href="mailto:jediknil@belkadan.com" target="_blank">jediknil@belkadan.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

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.<br>
<br>
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.<br>
<br>
Jordy<br>
<div><div><br>
<br>
On May 26, 2012, at 15:30, Alexander Kornienko wrote:<br>
<br>
> Hello llvm-commits, clang-commits,<br>
><br>
> 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;<br>


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


><br>
> I can't guarantee that all these 6 locations are real bugs, but they look very much like unintended fall-throughs.<br>
><br>
> Could you, please, review this patch?<br>
> Thanks!<br>
><br>
> 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?<br>
> 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:<br>


> #ifdef __clang__<br>
> #if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")<br>
> #define FALLTHROUGH [[clang::fallthrough]]<br>
> #endif<br>
> #endif<br>
><br>
> #ifndef FALLTHROUGH<br>
> #define FALLTHROUGH do { } while (0)<br>
> #endif<br>
><br>
><br>
> --<br>
> Best regards,<br>
> Alexander Kornienko</div></div></blockquote></div></div></div></div></blockquote><div> </div>-- <br>Best regards,<br><div class="HOEnZb"><div class="h5"><div>Alexander Kornienko</div>
</div></div></div><br><br>