[LLVMdev] [llvm-commits] PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

[Alexander Kornienko]

Ping.

On Fri, Jul 27, 2012 at 3:42 AM, Alexander Kornienko <alexfh at google.com>wrote:

> On Fri, Jul 27, 2012 at 2:02 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> On Thu, Jul 26, 2012 at 4:24 PM, Jim Grosbach <grosbach at apple.com> wrote:
>>
>>>
>>> On Jul 26, 2012, at 4:07 PM, Chris Lattner <clattner at apple.com> wrote:
>>>
>>> > <dropping llvm-commits>
>>> >
>>> > On Jul 2, 2012, at 9:59 AM, Alexander Kornienko wrote:
>>> >
>>> >> Hi llvmdev, llvm-commits,
>>> >>
>>> >> There was a discussion on this topic a while ago, and now I've
>>> decided to make a formal proposal and post it here.
>>> >
>>> > I missed the earlier discussion, so I'm sorry for chiming in late.
>>> >
>>> >> I propose to add the LLVM_FALLTHROUGH macro for specifying intended
>>> fall-through locations between switch cases.
>>> >
>>> > I don't really see that the tradeoff here is worthwhile.  It is
>>> possible that we have some fallthrough bugs, but the cost of sprinkling
>>> this macro everywhere doesn't seem like the right tradeoff.
>>> >
>>>
>>> While I tend to agree with you, it's also true that for many (most?) of
>>> the locations where we have an intentional fall through, there's already a
>>> comment to that effect as a matter of style. This would simply formalize
>>> that currently voluntary bit of style and use the macro rather than a
>>> comment. Depending on how many bugs this would enable find (perhaps some
>>> experiments are in order?), I could be convinced the tradeoff is worthwhile.
>>
>>
>> I believe Alex has already gone through the hits from this diagnostic;
>> see the fallthrough-bugs-llvm.diff patch attached to the original mail on
>> this thread for the bugs he found.
>>
>> Alex: can you tell us how many FALLTHROUGH annotations would be required,
>> and how many bugs you found, when running this over LLVM and Clang?
>>
> Two months ago it was like this:
>
> 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.
>
>
> The patch with fixes is attached, it's unchecked, not reviewed, and most
> probably out-of-sync with current HEAD. But it shows the idea of how these
> bugs look like, and that it's not always easy to spot them manually. The
> number of actual bugs I found (5 in LLVM + 1 in Clang) isn't overwhelming,
> but I'm sure that many of similar bugs never got to the repository, but
> took enough debugging time to be found.
>
> If the idea seems to be not bad in general, I can do all the comments to
> macro conversion routine, it should be totally about 400 single line
> changes, which doesn't seem to be a great problem for reviewers. As for me,
> I'm going to come up with a tool for mostly automatic transition.
>
> --
> Best regards,
> Alexander Kornienko
>


-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120809/aa660b74/attachment.html>