Hi Chandler,<div><br></div><div>Here's the new patch. Changes in short: clang:: prefix for the attribute, avoid fix-it hint when some fall-through paths are already annotated, std::map -> llvm::SmallPtrSet, fixed indentation issues and minor wording changes in comments.<br>
<br><div class="gmail_quote">On Thu, Apr 26, 2012 at 1:09 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><div>I think we should leave C++98 support for some later contributor to add if they are motivated to get this feature.</div></div></div></blockquote><div>Yep, that's the way to go. </div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> case 223: // expected-warning{{unannotated fall-through between switch labels}}<br>
> [[fallthrough]]; // expected-warning{{fallthrough annotation does not directly precede switch label}}<br>
> }<br>
> return t;<br>
> }<br>
<br>
I'm not convinced the last case should warn. It's pretty common to put a 'break;' in the last case statement (especially a non-default one) for symmetry, and to defend against further changes. While it's a little less conceivable that you would want to imply [[fallthrough]]; for future changes, it still seems like a valid case for symmetry.<br>
</blockquote></div><div>I thought of fall-through annotation as a last resort for situations when having fall-through is critical for performance or other (not really frequent) serious reasons. For me it's a <font face="courier new, monospace">goto</font>-style thing, which should be avoided in most cases.</div>
</div></div></blockquote><div><br></div></div><div>My problem with this is that the fallthrough actually *looks* less goto-style. I would go further, I think we should treat the last case as-if it had a case following it.</div>
</div></div></blockquote><div>Currently I have no idea how to implement this specific restriction. I would suggest to leave it for one of the next iterations.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><div>Consider that a common use case is making huge switches with many case labels future proof to cases being moved or re-sorted. Without insisting on either 'break;' or '[[fallthrough]];' here, the last case becomes specially ignored by the warning. With a warning in the reverse, the last case grows an inverted requirement from every other case.</div>
<div class="im">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Even if we stick with allowing [[fallthrough]], should we /also/ allow [[clang:fallthrough]]?<br></blockquote></div><div>Not a bad idea, and it's quite popular in this discussion ;) I think I'll do this in the next iteration.</div>
</div></div></blockquote><div><br></div></div><div>FWIW, Richard convinced me that this must be spelled 'clang::fallthrough' for the immediate future. We should probably focus on that form.</div></div></div></blockquote>
<div>Done. It turned out though, that namespaces for attributes were not used until now, so I implemented my own vision: I just add namespace as a prefix to spelling with triple underscore delimiter. Better ideas?</div><div>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div class="im"><div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><div><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Rather than build a map of false/true pairs, then mark half of them true, why not use an llvm::SmallPtrSet, building up the set in VisitAttributedStmt and removing from it in MarkFallthroughValid? Advantages: stack-based allocation if the number of fallthrough statements is small, only one data structure, and no second traversal and no copying when it comes time to warn about the invalid fallthroughs.<br>
</blockquote></div></div><div>I'll look into this.</div></div></div></blockquote><div><br></div></div><div>I think any of these data structure concerns should be addressed even in the first iteration fwiw.</div></div>
</div></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This is a deal-breaker for me; fixits should always create buildable code. I'd be okay with this being fixed to add braces, or with the fallthrough statement being pushed outside the if. It would also be nice to have a set of fixit tests, to make sure we're not just warning but actually doing the right thing.<br>
</blockquote></div><div>I currently removed all artificial intelligence from this code, so it just suggests to insert a fall-through annotation just before a case label. This is more consistent (but it could also add warnings in a case like this:</div>
</div></div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace">case 1:</font></div></div></div><div class="gmail_extra">
<div class="gmail_quote"><div><font face="courier new, monospace"> if (x) {</font></div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace"> ...;</font></div></div></div>
<div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace"> [[fallthrough]];</font></div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace"> } else {</font></div>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace"> ...;</font></div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace"> }</font></div>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace">case 2:</font></div></div></div></blockquote><div class="gmail_extra"><div class="gmail_quote"><div>because adding an annotation before case 2 would render a previous annotation inside if(x) {..} to be redundant. But this is not a broken build, it's just a warning which doesn't affect semantics.</div>
</div></div></blockquote><div><br></div></div><div>I don't understand why we can't simply suppress the fixit note when we have a case like this one that it might be wrong for. Saying it's "just a warning" doesn't help much if you use -Werror. My suggestion is to not suggest a fixit hint if there are already [[fallthrough]] annotations in the case somewhere, we're almost certain to get it wrong.</div>
</div></div></blockquote><div>You're right. Now there's no fix-it hint when some fall-through paths to the same case label are already annotated.</div><div> </div><div>-- </div><div>Best regards,</div><div>Alexander Kornienko</div>
</div><br>
</div>