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

[Alexander Kornienko]

Ping.

On Thu, Aug 9, 2012 at 3:04 PM, Alexander Kornienko <alexfh at google.com>wrote:

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


-- 
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-dev/attachments/20120823/a172e426/attachment.html>