<div class="gmail_extra">Hello,<br><br><div class="gmail_quote">On Tue, Apr 24, 2012 at 6:37 PM, Jordy Rose <span dir="ltr"><<a href="mailto:jediknil@belkadan.com" target="_blank">jediknil@belkadan.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">A couple more comments from me, though I'll be glad to have this in trunk soon!<br>
<div class="im"><br>
> test.cpp:19:5: warning: unannotated fall-through between switch labels; use [[fallthrough]]; attribute to silence this warning [-Wimplicit-fallthrough]<br>
> case 5:<br>
> ^<br>
> test.cpp:18:9: note: fall-through from here; use [[fallthrough]]; attribute to silence this warning [-Wimplicit-fallthrough]<br>
> ;<br>
> ^<br>
> [[fallthrough]];<br>
<br>
</div>I'm still of the opinion that the primary warning shouldn't mention how to silence the warning, even if it means [[fallthrough]] is less discoverable. As for the note, does it bother anyone else that "[[fallthrough]];" is not an attribute? This goes with Matthieu's complaint: what we've really done is invented a new statement type that's implemented using attribute syntax.<br>
</blockquote><div>I've changed diagnostic message, details in my previous e-mail.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
That doesn't mean I don't like it, because I do. But it does make the diagnostic a bit weird. How about "add '[[fallthrough]];' to silence this warning"?<br></blockquote><div>Diagnostic note with fix-it hint is now almost like this.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
And again I think the fixit (and perhaps the message) should use GCC __attribute__ syntax if we're not in C++11 mode.<br></blockquote><div>As Richard noted, GCC syntax doesn't allow statement attributes. So if there's a great demand for this feature in C++ 98 mode, it would make sense to either add statement attribute support as an extension, or implement this using other features, e.g.:</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"> ...;</font></div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace"> #pragma fallthrough</font></div>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace">case 2:</font></div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace">...</font></div>
</div></div></blockquote><div class="gmail_extra"><div class="gmail_quote"><div>or introduce an attribute and mark a special function with it:</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">inline void __fallthrough__() __attribute__(fallthrough_marker) {}</font></div></div></div></blockquote><div class="gmail_extra"><div class="gmail_quote">
<div>and then use it as a fall-through annotation:</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"> ...;</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">case 2:</font></div></div></div><div class="gmail_extra"><div class="gmail_quote"><div><font face="courier new, monospace">...</font></div>
</div></div></blockquote><div class="gmail_extra"><div class="gmail_quote"><div>Or maybe someone really interested in supporting this feature in C++ 98 mode could offer any better idea?</div><div><br></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>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. Think of it as a kind of #pragma warning(disable...) stuff. So I wouldn't relax this diagnostic, as it would be a kind of encouraging to use this annotation more frequently. This is my point, but it would be interesting to listen to someone having other opinion.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> [[fallthrough]] // no semicolon<br>
> case 2:<br>
> doSomethingElse(z);<br>
> break;<br>
> }<br>
<br>
Should we have a fixit here that just adds a semicolon? ("Perhaps you meant..."-type thing.) Since we already know the attribute target is a LabelStmt within a switch, we know it should be valid to add the semicolon. If we want to be very smart, we could do it only when the attribute is on a different line and was probably supposed to be a null statement anyway.<br>
</blockquote><div>It sounds reasonable. I'll try to implement this in the next iteration.</div><div> </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>Not a bad idea, and it's quite popular in this discussion ;) I think I'll do this in the next iteration.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> class FallthroughMapper : public RecursiveASTVisitor<FallthroughMapper> {<br>
> bool foundSwitchStatements;<br>
> // maps statements with fallthrough attributes to 'placed in a correct<br>
> // position' flag<br>
> typedef std::map<const AttributedStmt*, bool> AttrStmtMap;<br>
> AttrStmtMap fallthroughStmts;<br>
> size_t validFallthroughs;<br>
><br>
> public:<br>
> FallthroughMapper()<br>
> : foundSwitchStatements(false),<br>
> validFallthroughs(0) {<br>
> }<br>
><br>
> bool FoundSwitchStatements() const { return foundSwitchStatements; }<br>
><br>
> void MarkFallthroughValid(const AttributedStmt *stmt) {<br>
> AttrStmtMap::iterator it = fallthroughStmts.find(stmt);<br>
> assert(it != fallthroughStmts.end());<br>
> assert(!it->second);<br>
> assert(validFallthroughs < fallthroughStmts.size());<br>
> ++validFallthroughs;<br>
> it->second = true;<br>
> }<br>
><br>
> typedef std::vector<const AttributedStmt*> StmtVec;<br>
> StmtVec GetInvalidFallthroughs() const {<br>
> StmtVec res(fallthroughStmts.size() - validFallthroughs);<br>
> if (!res.empty()) {<br>
> res.resize(0);<br>
> for (AttrStmtMap::const_iterator it = fallthroughStmts.begin(),<br>
> end = fallthroughStmts.end();<br>
> it != end; ++it) {<br>
> if (!it->second)<br>
> res.push_back(it->first);<br>
> }<br>
> }<br>
> return res;<br>
> }<br>
><br>
> // RecursiveASTVisitor setup<br>
> bool shouldWalkTypesOfTypeLocs() const { return false; }<br>
><br>
> bool VisitAttributedStmt(AttributedStmt *S) {<br>
> if (ContainsFallThrough(S))<br>
> fallthroughStmts[S] = false;<br>
> return true;<br>
> }<br>
><br>
> bool VisitSwitchStmt(SwitchStmt *S) {<br>
> foundSwitchStatements = true;<br>
> return true;<br>
> }<br>
> };<br>
<br>
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>I'll look into this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, there seems to be an extra level of indentation here.</blockquote><div>Yep, thanks for noting this.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> // FIXME: we need to provide generic support of attributes with C++ 0x syntax<br>
<br>
C++11. :-)</blockquote><div>Sure ;) </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> More known issues:<br>...</div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">> * fix-it hints do not take in account necessity of adding a curly braces when fallthrough annotation is suggested in a place where only one statement is allowed:<br>
> if(x) ...; [[fallthrough]]; else return 13;<br>
> here fix-it hint should have suggested something like this:<br>
> if(x) { ...; [[fallthrough]]; } else return 13;<br>
<br>
</div>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>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><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Sorry for the deluge of comments, but we all want this to be as solid as can be when we ship it, right? :-) You're doing good work; thank you.</blockquote><div>Yep, we all want it to be solid, but some also want it to be cleverer than 90% of C++ developers.</div>
</div><div>Thanks for the comments!</div><div><br></div>-- <br>
</div><div class="gmail_extra">Best regards,</div><div class="gmail_extra">Alexander Kornienko</div>