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

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.

-Jim

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120726/ca662685/attachment.html>

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120727/8899c4a3/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-dev/attachments/20120727/8899c4a3/attachment.obj>