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

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 propose to add the LLVM_FALLTHROUGH macro for specifying intended
fall-through locations between switch cases.

*INTRODUCTION*

The switch construct of C/C++ languages allows fall-throughs between switch
labels when control flow is not directed elsewhere by a break, return or
continue statement or other ways. There are certain coding idioms which
utilize this behavior, e.g. Duff's
device<http://en.wikipedia.org/wiki/Duff's_device>,
but much more frequently the use of switch statements doesn't involve a
fall-through. (A commonly used case - when several switch labels mark one
statement - is not considered a fall-through here.) As there's no special
way to mark a fall-through, it's relatively easy to make a mistake by just
missing a break/return statement before the next switch label. This kind of
mistake is sometimes difficult to spot by manual code examination, and the
compiler can't provide any help either. It's common to mark fall-through
locations with a comment, but parsing comments would incur a significant
performance penalty and is probably an unhealthy approach overall. So the
use of comment-only annotations is limited to manual inspections.

Some time ago I've added the 'Unintended fall-through in switch
statement'
diagnostics to clang (
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120430/057180.html
).

This is a proposal to introduce clang's unintended fall-through diagnostic
to llvm/clang code.

*DESCRIPTION OF 'UNINTENDED FALL-THROUGH IN SWITCH STATEMENTS'
DIAGNOSTIC
IN CLANG*

Related functionality in clang introduces several diagnostic messages and a
syntax for intended fall-through annotation, based on C++11 attributes
([dcl.attr],
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3376.pdf, 7.6).

The [[clang::fallthrough]] attribute should be applied to a null statement
placed in a point of execution where fall-through to the next switch label
occurs, e.g.:

switch (n) {
  case 0:
    ...  // no break here
  case 1:  // warning: unannotated fall-through between switch labels
    if (x)
      break;
    else if (y) {
      ...
      *[[clang::fallthrough]];*  // annotated fall-through to case 2
    }
    else
      return 13;
  case 2:  // no warning here
    ...
    *[[clang::fallthrough]];*  // annotated fall-through to case 3
  case 3:
    ...
}

So, the [[clang::fallthrough]]; annotation can be used in mostly in the
same way as break; or return; statements (but it doesn't change
control-flow, it just annotates a fall-through). The following rules are
checked for this annotation:
  * the clang::fallthrough attribute can only be attached to a
null-statement;
  * the [[clang::fallthrough]]; annotation should be placed inside a switch
 body;
  * it should be placed on an execution path between any statement inside a
switch body and a case/default label (this means there *is* a fall-through
to the corresponding case/default label);
  * no statements should exist on an execution path between the
[[clang::fallthrough]]; annotation and the next case/default label.

*PROPOSAL FOR LLVM/CLANG CODE*

I ran the unintended fall-through diagnostics against llvm/clang code and
found a number of fall-through locations, most of them are probably
intended. But some look like bugs (see the attached *
fallthrough-bugs-llvm.diff* file).

To start using the proposed diagnostics code has to be prepared.

First, we need to mark all intended fall-through locations with the
clang::fallthrough attribute. It makes sense to 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 the macro (see *
fallthrough-macro.diff*):
*#ifdef __clang__
**#if __has_feature(cxx_attributes) &&
__has_warning("-Wimplicit-fallthrough")
*
*#define LLVM_FALLTHROUGH [[clang::fallthrough]]
**#endif
**#endif
****#ifndef LLVM_FALLTHROUGH
**#define LLVM_FALLTHROUGH do { } while (0)
**#endif*

After this we can start using *-Wimplicit-fallthrough*.

Please express your opinions on this proposal.

I'm also ready to provide diffs for marking all fall-through locations with
the LLVM_FALLTHROUGH macro, if this proposal gets approval.

-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120702/e3b7093a/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/20120702/e3b7093a/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fallthrough-macro.diff
Type: application/octet-stream
Size: 1798 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120702/e3b7093a/attachment-0001.obj>

Is anyone interested in introducing fall-through annotations and the
related diagnostic in LLVM/Clang code?


On Mon, Jul 2, 2012 at 6:59 PM, Alexander Kornienko <alexfh at
google.com>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 propose to add the LLVM_FALLTHROUGH macro for specifying intended
> fall-through locations between switch cases.
>
> *INTRODUCTION*
>
> The switch construct of C/C++ languages allows fall-throughs between
> switch labels when control flow is not directed elsewhere by a break,
> return or continue statement or other ways. There are certain coding
> idioms which utilize this behavior, e.g. Duff's
device<http://en.wikipedia.org/wiki/Duff's_device>,
> but much more frequently the use of switch statements doesn't involve a
> fall-through. (A commonly used case - when several switch labels mark one
> statement - is not considered a fall-through here.) As there's no
special
> way to mark a fall-through, it's relatively easy to make a mistake by
just
> missing a break/return statement before the next switch label. This kind of
> mistake is sometimes difficult to spot by manual code examination, and the
> compiler can't provide any help either. It's common to mark
fall-through
> locations with a comment, but parsing comments would incur a significant
> performance penalty and is probably an unhealthy approach overall. So the
> use of comment-only annotations is limited to manual inspections.
>
> Some time ago I've added the 'Unintended fall-through in switch
statement'
> diagnostics to clang (
>
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120430/057180.html
> ).
>
> This is a proposal to introduce clang's unintended fall-through
diagnostic
> to llvm/clang code.
>
> *DESCRIPTION OF 'UNINTENDED FALL-THROUGH IN SWITCH STATEMENTS'
DIAGNOSTIC
> IN CLANG*
>
> Related functionality in clang introduces several diagnostic messages and
> a syntax for intended fall-through annotation, based on C++11 attributes
> ([dcl.attr],
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3376.pdf, 7.6).
>
> The [[clang::fallthrough]] attribute should be applied to a null
> statement placed in a point of execution where fall-through to the next
> switch label occurs, e.g.:
>
> switch (n) {
>   case 0:
>     ...  // no break here
>   case 1:  // warning: unannotated fall-through between switch labels
>     if (x)
>       break;
>     else if (y) {
>       ...
>       *[[clang::fallthrough]];*  // annotated fall-through to case 2
>     }
>     else
>       return 13;
>   case 2:  // no warning here
>     ...
>     *[[clang::fallthrough]];*  // annotated fall-through to case 3
>   case 3:
>     ...
> }
>
> So, the [[clang::fallthrough]]; annotation can be used in mostly in the
> same way as break; or return; statements (but it doesn't change
> control-flow, it just annotates a fall-through). The following rules are
> checked for this annotation:
>   * the clang::fallthrough attribute can only be attached to a
> null-statement;
>   * the [[clang::fallthrough]]; annotation should be placed inside a
> switch body;
>   * it should be placed on an execution path between any statement inside
> a switch body and a case/default label (this means there *is* a
> fall-through to the corresponding case/default label);
>   * no statements should exist on an execution path between the
> [[clang::fallthrough]]; annotation and the next case/default label.
>
> *PROPOSAL FOR LLVM/CLANG CODE*
>
> I ran the unintended fall-through diagnostics against llvm/clang code and
> found a number of fall-through locations, most of them are probably
> intended. But some look like bugs (see the attached *
> fallthrough-bugs-llvm.diff* file).
>
> To start using the proposed diagnostics code has to be prepared.
>
> First, we need to mark all intended fall-through locations with the
> clang::fallthrough attribute. It makes sense to 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 the
> macro (see *fallthrough-macro.diff*):
> *#ifdef __clang__
> **#if __has_feature(cxx_attributes) &&
> __has_warning("-Wimplicit-fallthrough")
> *
> *#define LLVM_FALLTHROUGH [[clang::fallthrough]]
> **#endif
> **#endif
> ****#ifndef LLVM_FALLTHROUGH
> **#define LLVM_FALLTHROUGH do { } while (0)
> **#endif*
>
> After this we can start using *-Wimplicit-fallthrough*.
>
> Please express your opinions on this proposal.
>
> I'm also ready to provide diffs for marking all fall-through locations
> with the LLVM_FALLTHROUGH macro, if this proposal gets approval.
>
> --
> 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/20120726/05ca15b5/attachment.html>

On Thu, Jul 26, 2012 at 8:53 PM, Cameron McInally
<cameron.mcinally at nyu.edu>wrote:
> Hey Alex,
>
> Sorry if this is a silly question... are you asking if anyone "wants
the
> functionality proposed" or "wants to write the code for the
functionality
> proposed"?
>
*-Wimplicit-fallthrough* diagnostic is already implemented, and the patch
in this thread is a portability macro for
*[[clang::fallthrough]]*attribute (which is also implemented, of
course). I only need an approval
from community to add this macro and replace comment-only fall-through
annotations with the *LLVM_FALLTHROUGH;* macro.

> -Cameron
>
> On Thu, Jul 26, 2012 at 2:17 PM, Alexander Kornienko <alexfh at
google.com>wrote:
>
>> Is anyone interested in introducing fall-through annotations and the
>> related diagnostic in LLVM/Clang code?
>>
>>
>> On Mon, Jul 2, 2012 at 6:59 PM, Alexander Kornienko <alexfh at
google.com>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 propose to add the LLVM_FALLTHROUGH macro for specifying intended
>>> fall-through locations between switch cases.
>>>
>>> *INTRODUCTION*
>>>
>>> The switch construct of C/C++ languages allows fall-throughs
between
>>> switch labels when control flow is not directed elsewhere by a
break,
>>> return or continue statement or other ways. There are certain
coding
>>> idioms which utilize this behavior, e.g. Duff's
device<http://en.wikipedia.org/wiki/Duff's_device>,
>>> but much more frequently the use of switch statements doesn't
involve a
>>> fall-through. (A commonly used case - when several switch labels
mark one
>>> statement - is not considered a fall-through here.) As there's
no special
>>> way to mark a fall-through, it's relatively easy to make a
mistake by just
>>> missing a break/return statement before the next switch label. This
kind of
>>> mistake is sometimes difficult to spot by manual code examination,
and the
>>> compiler can't provide any help either. It's common to mark
fall-through
>>> locations with a comment, but parsing comments would incur a
significant
>>> performance penalty and is probably an unhealthy approach overall.
So the
>>> use of comment-only annotations is limited to manual inspections.
>>>
>>> Some time ago I've added the 'Unintended fall-through in
switch
>>> statement' diagnostics to clang (
>>>
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120430/057180.html
>>> ).
>>>
>>> This is a proposal to introduce clang's unintended fall-through
>>> diagnostic to llvm/clang code.
>>>
>>> *DESCRIPTION OF 'UNINTENDED FALL-THROUGH IN SWITCH
STATEMENTS'
>>> DIAGNOSTIC IN CLANG*
>>>
>>> Related functionality in clang introduces several diagnostic
messages
>>> and a syntax for intended fall-through annotation, based on C++11
>>> attributes ([dcl.attr],
>>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3376.pdf,
7.6).
>>>
>>> The [[clang::fallthrough]] attribute should be applied to a null
>>> statement placed in a point of execution where fall-through to the
next
>>> switch label occurs, e.g.:
>>>
>>> switch (n) {
>>>   case 0:
>>>     ...  // no break here
>>>   case 1:  // warning: unannotated fall-through between switch
labels
>>>     if (x)
>>>       break;
>>>     else if (y) {
>>>       ...
>>>       *[[clang::fallthrough]];*  // annotated fall-through to case
2
>>>     }
>>>     else
>>>       return 13;
>>>   case 2:  // no warning here
>>>     ...
>>>     *[[clang::fallthrough]];*  // annotated fall-through to case 3
>>>   case 3:
>>>     ...
>>> }
>>>
>>> So, the [[clang::fallthrough]]; annotation can be used in mostly in
the
>>> same way as break; or return; statements (but it doesn't change
>>> control-flow, it just annotates a fall-through). The following
rules are
>>> checked for this annotation:
>>>   * the clang::fallthrough attribute can only be attached to a
>>> null-statement;
>>>   * the [[clang::fallthrough]]; annotation should be placed inside
a
>>> switch body;
>>>   * it should be placed on an execution path between any statement
>>> inside a switch body and a case/default label (this means there
*is* a
>>> fall-through to the corresponding case/default label);
>>>   * no statements should exist on an execution path between the
>>> [[clang::fallthrough]]; annotation and the next case/default label.
>>>
>>> *PROPOSAL FOR LLVM/CLANG CODE*
>>>
>>> I ran the unintended fall-through diagnostics against llvm/clang
code
>>> and found a number of fall-through locations, most of them are
probably
>>> intended. But some look like bugs (see the attached *
>>> fallthrough-bugs-llvm.diff* file).
>>>
>>> To start using the proposed diagnostics code has to be prepared.
>>>
>>> First, we need to mark all intended fall-through locations with the
>>> clang::fallthrough attribute. It makes sense to 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
the
>>> macro (see *fallthrough-macro.diff*):
>>> *#ifdef __clang__
>>> **#if __has_feature(cxx_attributes) &&
>>> __has_warning("-Wimplicit-fallthrough")
>>> *
>>> *#define LLVM_FALLTHROUGH [[clang::fallthrough]]
>>> **#endif
>>> **#endif
>>> ****#ifndef LLVM_FALLTHROUGH
>>> **#define LLVM_FALLTHROUGH do { } while (0)
>>> **#endif*
>>>
>>> After this we can start using *-Wimplicit-fallthrough*.
>>>
>>> Please express your opinions on this proposal.
>>>
>>> I'm also ready to provide diffs for marking all fall-through
locations
>>> with the LLVM_FALLTHROUGH macro, if this proposal gets approval.
>>>
>>> --
>>> Best regards,
>>> Alexander Kornienko
>>>
>>
-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120727/a1e49c93/attachment.html>

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

-Chris

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