[cfe-commits] [PATCH] Implicit fall-through between switch labels
Alexander Kornienko
alexfh at google.com
Wed Apr 25 15:05:23 PDT 2012
Hello,
On Tue, Apr 24, 2012 at 6:37 PM, Jordy Rose <jediknil at belkadan.com> wrote:
> A couple more comments from me, though I'll be glad to have this in trunk
> soon!
>
> > test.cpp:19:5: warning: unannotated fall-through between switch labels;
> use [[fallthrough]]; attribute to silence this warning
> [-Wimplicit-fallthrough]
> > case 5:
> > ^
> > test.cpp:18:9: note: fall-through from here; use [[fallthrough]];
> attribute to silence this warning [-Wimplicit-fallthrough]
> > ;
> > ^
> > [[fallthrough]];
>
> 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.
>
I've changed diagnostic message, details in my previous e-mail.
> 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"?
>
Diagnostic note with fix-it hint is now almost like this.
> And again I think the fixit (and perhaps the message) should use GCC
> __attribute__ syntax if we're not in C++11 mode.
>
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.:
case 1:
...;
#pragma fallthrough
case 2:
...
or introduce an attribute and mark a special function with it:
inline void __fallthrough__() __attribute__(fallthrough_marker) {}
and then use it as a fall-through annotation:
case 1:
...;
__fallthrough__();
case 2:
...
Or maybe someone really interested in supporting this feature in C++ 98
mode could offer any better idea?
> case 223: // expected-warning{{unannotated fall-through
> between switch labels}}
> > [[fallthrough]]; // expected-warning{{fallthrough annotation does
> not directly precede switch label}}
> > }
> > return t;
> > }
>
> 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.
>
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 goto-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.
> > [[fallthrough]] // no semicolon
> > case 2:
> > doSomethingElse(z);
> > break;
> > }
>
> 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.
>
It sounds reasonable. I'll try to implement this in the next iteration.
> Even if we stick with allowing [[fallthrough]], should we /also/ allow
> [[clang:fallthrough]]?
>
Not a bad idea, and it's quite popular in this discussion ;) I think I'll
do this in the next iteration.
> class FallthroughMapper : public RecursiveASTVisitor<FallthroughMapper> {
> > bool foundSwitchStatements;
> > // maps statements with fallthrough attributes to 'placed in a
> correct
> > // position' flag
> > typedef std::map<const AttributedStmt*, bool> AttrStmtMap;
> > AttrStmtMap fallthroughStmts;
> > size_t validFallthroughs;
> >
> > public:
> > FallthroughMapper()
> > : foundSwitchStatements(false),
> > validFallthroughs(0) {
> > }
> >
> > bool FoundSwitchStatements() const { return foundSwitchStatements; }
> >
> > void MarkFallthroughValid(const AttributedStmt *stmt) {
> > AttrStmtMap::iterator it = fallthroughStmts.find(stmt);
> > assert(it != fallthroughStmts.end());
> > assert(!it->second);
> > assert(validFallthroughs < fallthroughStmts.size());
> > ++validFallthroughs;
> > it->second = true;
> > }
> >
> > typedef std::vector<const AttributedStmt*> StmtVec;
> > StmtVec GetInvalidFallthroughs() const {
> > StmtVec res(fallthroughStmts.size() - validFallthroughs);
> > if (!res.empty()) {
> > res.resize(0);
> > for (AttrStmtMap::const_iterator it = fallthroughStmts.begin(),
> > end = fallthroughStmts.end();
> > it != end; ++it) {
> > if (!it->second)
> > res.push_back(it->first);
> > }
> > }
> > return res;
> > }
> >
> > // RecursiveASTVisitor setup
> > bool shouldWalkTypesOfTypeLocs() const { return false; }
> >
> > bool VisitAttributedStmt(AttributedStmt *S) {
> > if (ContainsFallThrough(S))
> > fallthroughStmts[S] = false;
> > return true;
> > }
> >
> > bool VisitSwitchStmt(SwitchStmt *S) {
> > foundSwitchStatements = true;
> > return true;
> > }
> > };
>
> 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.
>
I'll look into this.
> Also, there seems to be an extra level of indentation here.
Yep, thanks for noting this.
> // FIXME: we need to provide generic support of attributes with C++ 0x
> syntax
>
> C++11. :-)
Sure ;)
> More known issues:
> ...
>
> * 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:
> > if(x) ...; [[fallthrough]]; else return 13;
> > here fix-it hint should have suggested something like this:
> > if(x) { ...; [[fallthrough]]; } else return 13;
>
> 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.
>
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:
case 1:
if (x) {
...;
[[fallthrough]];
} else {
...;
}
case 2:
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.
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.
Yep, we all want it to be solid, but some also want it to be cleverer than
90% of C++ developers.
Thanks for the comments!
--
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120426/82e517d8/attachment.html>
More information about the cfe-commits
mailing list