Is anyone interested in introducing fall-through annotations and the related diagnostic in LLVM/Clang code?<div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 2, 2012 at 6:59 PM, 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 llvmdev, llvm-commits,<div><br></div><div>There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.<br>
<div><br></div><div>I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases. </div>
<div><br></div><div><div style="font-family:arial,sans-serif;font-size:13px"><b>INTRODUCTION</b></div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">
The <font face="courier new, monospace">switch</font> construct of C/C++ languages allows fall-throughs between switch labels when control flow is not directed elsewhere by a <font face="courier new, monospace">break</font>, <font face="courier new, monospace">return</font> or <font face="courier new, monospace">continue</font> statement or other ways. There are certain coding idioms which utilize this behavior, e.g. <a href="http://en.wikipedia.org/wiki/Duff's_device" target="_blank">Duff's device</a>, but much more frequently the use of <font face="courier new, monospace">switch</font> 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.</div>
</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">Some time ago I've added the 'Unintended fall-through in switch statement' diagnostics to clang (<a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120430/057180.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120430/057180.html</a>).</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">This is a proposal to introduce clang's unintended fall-through diagnostic to llvm/clang code.<br>
</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px"><div><div><div><b>DESCRIPTION OF 'UNINTENDED FALL-THROUGH IN SWITCH STATEMENTS' DIAGNOSTIC IN CLANG</b></div>
<div><br></div></div><div>Related functionality in clang introduces several diagnostic messages and a syntax for intended fall-through annotation, based on C++11 attributes ([dcl.attr], <a href="http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3376.pdf" target="_blank">http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3376.pdf</a>, 7.6).</div>
<div><div><br></div><div>The <font face="courier new, monospace">[[clang::fallthrough]]</font> 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.:</div>
<div><div><br></div><div><font face="courier new, monospace">switch (n) {</font></div><div><font face="courier new, monospace"> case 0:</font></div><div><font face="courier new, monospace"> ... // no break here</font></div>
<div><font face="courier new, monospace"> case 1: // warning: unannotated fall-through between switch labels</font></div><div><font face="courier new, monospace"> if (x)</font></div><div><font face="courier new, monospace"> break;</font></div>
<div><font face="courier new, monospace"> else if (y) {</font></div><div><font face="courier new, monospace"> ...</font></div><div><font face="courier new, monospace"> <b>[[clang::fallthrough]];</b> // annotated fall-through to case 2</font></div>
<div><font face="courier new, monospace"> }</font></div><div><font face="courier new, monospace"> else</font></div><div><font face="courier new, monospace"> return 13;</font></div><div><font face="courier new, monospace"> case 2: // no warning here</font></div>
<div><font face="courier new, monospace"> ...</font></div><div><font face="courier new, monospace"> <b>[[clang::fallthrough]];</b> // annotated fall-through to case 3</font></div><div><font face="courier new, monospace"> case 3:</font></div>
<div><font face="courier new, monospace"> ...</font></div><div><font face="courier new, monospace">}</font></div></div><div><br></div><div>So, the <font face="courier new, monospace">[[clang::fallthrough]];</font> annotation can be used in mostly in the same way as <font face="courier new, monospace">break;</font> or <font face="courier new, monospace">return;</font> statements (but it doesn't change control-flow, it just annotates a fall-through). The following rules are checked for this annotation:</div>
</div></div><div><div> * the <font face="courier new, monospace">clang::fallthrough</font> attribute can only be attached to a null-statement;</div><div> * the <font face="courier new, monospace">[[clang::fallthrough]];</font> annotation should be placed inside a <font face="courier new, monospace">switch</font> body;</div>
<div> * it should be placed on an execution path between any statement inside a <font face="courier new, monospace">switch</font> body and a <font face="courier new, monospace">case/default</font> label (this means there <i>is</i> a fall-through to the corresponding <font face="courier new, monospace">case/default</font> label);</div>
<div> * no statements should exist on an execution path between the <font face="courier new, monospace">[[clang::fallthrough]];</font> annotation and the next <font face="courier new, monospace">case</font>/<font face="courier new, monospace">default</font> label.</div>
</div><div><br></div></div><div><b style="font-family:arial,sans-serif;font-size:13px">PROPOSAL FOR LLVM/CLANG CODE</b></div><div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">
<font color="#000000">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 <b>fallthrough-bugs-llvm.diff</b> file).</font></div>
<div style="font-family:arial,sans-serif;font-size:13px"><font color="#000000"><br></font></div><div style="font-family:arial,sans-serif;font-size:13px"><font color="#000000">To start using the proposed diagnostics code has to be prepared.</font></div>
<div style="font-family:arial,sans-serif;font-size:13px"><font color="#000000"><br></font></div><div style="font-size:13px"><font color="#000000" style="font-family:arial,sans-serif">First, we need to mark all intended fall-through locations with the </font><font color="#000000" face="courier new, monospace">clang::fallthrough</font><font color="#000000" style="font-family:arial,sans-serif"> attribute. It makes sense to</font><span style><font face="arial, sans-serif"> wrap </font><font face="courier new, monospace">[[clang::fallthrough]]</font><font face="arial, sans-serif"> 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 </font><font face="arial, helvetica, sans-serif"><b>fallthrough-macro.diff</b></font><font face="arial, sans-serif">):</font></span></div>
<div style="font-family:arial,sans-serif;font-size:13px"><font color="#000000"><b>#ifdef __clang__<br></b><b>#if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")<br></b></font></div>
<font color="#000000"><b style="font-family:arial,sans-serif;font-size:13px">#define LLVM_FALLTHROUGH [[clang::fallthrough]]<br></b><b style="font-family:arial,sans-serif;font-size:13px">#endif<br></b><b style="font-family:arial,sans-serif;font-size:13px">#endif<br>
</b><b style="font-family:arial,sans-serif;font-size:13px"></b><b style="font-family:arial,sans-serif;font-size:13px">#ifndef LLVM_FALLTHROUGH<br></b><b style="font-family:arial,sans-serif;font-size:13px">#define LLVM_FALLTHROUGH do { } while (0)<br>
</b><b style="font-family:arial,sans-serif;font-size:13px">#endif</b></font><br clear="all"><div><br></div><div>After this we can start using <i>-Wimplicit-fallthrough</i>.</div><div><br></div><div>Please express your opinions on this proposal.</div>
<div><br></div><div>I'm also ready to provide diffs for marking all fall-through locations with the LLVM_FALLTHROUGH macro, if this proposal gets approval.</div><span class="HOEnZb"><font color="#888888"><div><br></div>
-- <br>
<div>Best regards,</div><div>Alexander Kornienko</div></font></span></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div><div><font color="#666666"><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(213,15,37);border-right-color:rgb(213,15,37);border-bottom-color:rgb(213,15,37);border-left-color:rgb(213,15,37);padding-top:2px;margin-top:2px">Alexander Kornienko |</span><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(51,105,232);border-right-color:rgb(51,105,232);border-bottom-color:rgb(51,105,232);border-left-color:rgb(51,105,232);padding-top:2px;margin-top:2px"> Software Engineer |</span></font><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(0,153,57);border-right-color:rgb(0,153,57);border-bottom-color:rgb(0,153,57);border-left-color:rgb(0,153,57);padding-top:2px;margin-top:2px"><font color="#666666"> </font><a href="mailto:alexfh@google.com" style="color:rgb(17,85,204)" target="_blank">alexfh@google.com</a> |</span><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(238,178,17);border-right-color:rgb(238,178,17);border-bottom-color:rgb(238,178,17);border-left-color:rgb(238,178,17);padding-top:2px;margin-top:2px"> <a value="+35315435283" style="color:rgb(17,85,204)">+49 151 221 77 957</a></span></div>
</div><div><font color="#666666"><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">Google Germany GmbH | </span><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">Dienerstr. 12 | </span><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">80331 München</span></font></div>
<br>
</div>