[cfe-commits] [PATCH] Implicit fall-through between switch labels

[Jordy Rose]

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.

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"?

And again I think the fixit (and perhaps the message) should use GCC __attribute__ syntax if we're not in C++11 mode.


> int fallthrough_position(int n) {
>   int t = n;
>   switch (n) {
>     case 221:
>       [[fallthrough]]; // expected-warning{{fallthrough annotation does not directly precede switch label}}
>       return 1;
>     case 222:
>       [[fallthrough]]; // expected-warning{{fallthrough annotation does not directly precede switch label}}
>       t += 400;    // expected-note{{fall-through from here}}
>     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.

This one isn't in your test cases, but I just thought of it:

> switch (x) {
> case 1:
>   doSomething(y);
>   [[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.


> def FallThrough : Attr {
>   let Spellings = ["fallthrough"];
>   let Namespaces = [""];
>   let Subjects = [CaseStmt, DefaultStmt];
> }

Even if we stick with allowing [[fallthrough]], should we /also/ allow [[clang:fallthrough]]?

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

Also, there seems to be an extra level of indentation here.


> // FIXME: we need to provide generic support of attributes with C++ 0x syntax

C++11. :-)


> More known issues:
>  * currently this code doesn't try to minimize or somehow optimize the number of fix-it hints, it just offers them in all execution paths containing fall-through after the last executable statement - this provides fine-grained diagnostic, but probably too verbose;
>  * 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.


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.

Jordy