[Sema] Make BreakContinueFinder handle nested loops.

[efriedma]

Summary

We don't care about break or continue statements that aren't
associated with the current loop, so make sure the visitor
doesn't find them.

Fixes https://bugs.llvm.org/show_bug.cgi?id=32648 .

[efriedma]

Ping

[rsmith]

LGTM, thanks!

I'm not particularly happy with the number of different places where we duplicate the knowledge of how break/continue bind to enclosing loops, but fixing that seems beyond the scope of this change.

lib/Sema/SemaStmt.cpp

1610 ↗

(On Diff #103761)

In CodeGen, we don't evaluate the condition within the continue/break scope of the loop... but I'm pretty sure that doesn't matter because only the range statement can contain a break or continue anyway.

cfe/trunk/lib/Sema/SemaStmt.cpp

if (CXXOperatorCallExpr *Call = dyn_cast<CXXOperatorCallExpr>(Statement)) {
       return DRE;
     }
 
     return false;
   }
 
   // A visitor to determine if a continue or break statement is a
   // subexpression.
-  class BreakContinueFinder : public EvaluatedExprVisitor<BreakContinueFinder> {
+  class BreakContinueFinder : public ConstEvaluatedExprVisitor<BreakContinueFinder> {
     SourceLocation BreakLoc;
     SourceLocation ContinueLoc;
+    bool InSwitch = false;
+
   public:
-    BreakContinueFinder(Sema &S, Stmt* Body) :
+    BreakContinueFinder(Sema &S, const Stmt* Body) :
         Inherited(S.Context) {
       Visit(Body);
     }
 
-    typedef EvaluatedExprVisitor<BreakContinueFinder> Inherited;
+    typedef ConstEvaluatedExprVisitor<BreakContinueFinder> Inherited;
 
-    void VisitContinueStmt(ContinueStmt* E) {
+    void VisitContinueStmt(const ContinueStmt* E) {
       ContinueLoc = E->getContinueLoc();
     }
 
-    void VisitBreakStmt(BreakStmt* E) {
+    void VisitBreakStmt(const BreakStmt* E) {
+      if (!InSwitch)
         BreakLoc = E->getBreakLoc();
     }
 
+    void VisitSwitchStmt(const SwitchStmt* S) {
+      if (const Stmt *Init = S->getInit())
+        Visit(Init);
+      if (const Stmt *CondVar = S->getConditionVariableDeclStmt())
+        Visit(CondVar);
+      if (const Stmt *Cond = S->getCond())
+        Visit(Cond);
+
+      // Don't return break statements from the body of a switch.
+      InSwitch = true;
+      if (const Stmt *Body = S->getBody())
+        Visit(Body);
+      InSwitch = false;
+    }
+
+    void VisitForStmt(const ForStmt *S) {
+      // Only visit the init statement of a for loop; the body
+      // has a different break/continue scope.
+      if (const Stmt *Init = S->getInit())
+        Visit(Init);
+    }
+
+    void VisitWhileStmt(const WhileStmt *) {
+      // Do nothing; the children of a while loop have a different
+      // break/continue scope.
+    }
+
+    void VisitDoStmt(const DoStmt *) {
+      // Do nothing; the children of a while loop have a different
+      // break/continue scope.
+    }
+
+    void VisitCXXForRangeStmt(const CXXForRangeStmt *S) {
+      // Only visit the initialization of a for loop; the body
+      // has a different break/continue scope.
+      if (const Stmt *Range = S->getRangeStmt())
+        Visit(Range);
+      if (const Stmt *Begin = S->getBeginStmt())
+        Visit(Begin);
+      if (const Stmt *End = S->getEndStmt())
+        Visit(End);
+    }
+
+    void VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S) {
+      // Only visit the initialization of a for loop; the body
+      // has a different break/continue scope.
+      if (const Stmt *Element = S->getElement())
+        Visit(Element);
+      if (const Stmt *Collection = S->getCollection())
+        Visit(Collection);
+    }
+
     bool ContinueFound() { return ContinueLoc.isValid(); }
     bool BreakFound() { return BreakLoc.isValid(); }
     SourceLocation GetContinueLoc() { return ContinueLoc; }
     SourceLocation GetBreakLoc() { return BreakLoc; }
 
   };  // end class BreakContinueFinder
 
   // Emit a warning when a loop increment/decrement appears twice per loop

cfe/trunk/test/Sema/loop-control.c

 }
 
 void pr8880_23(int x, int y) {
   switch(x) {
   case 1:
     for ( ; ({ ++y; break; y;}); ++y) {} // expected-warning{{'break' is bound to loop, GCC binds it to switch}}
   }
 }
+
+void pr32648_1(int x, int y) {
+  switch(x) {
+  case 1:
+    for ( ; ({ ++y; switch (y) { case 0: break; } y;}); ++y) {} // no warning
+  }
+}
+
+void pr32648_2(int x, int y) {
+  while(x) {
+    for ( ; ({ ++y; switch (y) { case 0: continue; } y;}); ++y) {}  // expected-warning {{'continue' is bound to current loop, GCC binds it to the enclosing loop}}
+  }
+}
+
+void pr32648_3(int x, int y) {
+  switch(x) {
+  case 1:
+    for ( ; ({ ++y; for (; y; y++) { break; } y;}); ++y) {} // no warning
+  }
+}
+
+void pr32648_4(int x, int y) {
+  switch(x) {
+  case 1:
+    for ( ; ({ ++y; for (({ break; }); y; y++) { } y;}); ++y) {} // expected-warning{{'break' is bound to loop, GCC binds it to switch}}
+  }
+}
+
+void pr32648_5(int x, int y) {
+  switch(x) {
+  case 1:
+    for ( ; ({ ++y; while (({ break; y; })) {} y;}); ++y) {} // expected-warning{{'break' is bound to current loop, GCC binds it to the enclosing loop}}
+  }
+}
+
+void pr32648_6(int x, int y) {
+  switch(x) {
+  case 1:
+    for ( ; ({ ++y; do {} while (({ break; y; })); y;}); ++y) {} // expected-warning{{'break' is bound to current loop, GCC binds it to the enclosing loop}}
+  }
+}
+
+void pr32648_7(int x, int y) {
+  switch(x) {
+  case 1:
+    for ( ; ({ ++y; do { break; } while (y); y;}); ++y) {} // no warning
+  }
+}

cfe/trunk/test/SemaCXX/warn-loop-analysis.cpp

void test7() {
   for (;;i++) {
     if (true) continue;
     i++;
   }
   for (;;i--) {
     if (true) continue;
     i--;
   }
+
+  // But do warn if the continue is in a nested loop.
+  for (;;i--) { // expected-note{{decremented here}}
+    for (int j = 0; j < 10; ++j) continue;
+    i--; // expected-warning{{decremented both}}
+  }
 }
 
 struct iterator {
   iterator operator++() { return *this; }
   iterator operator++(int) { return *this; }
   iterator operator--() { return *this; }
   iterator operator--(int) { return *this; }
 };
void test8() {
   for (;;i++) {
     if (true) continue;
     i++;
   }
   for (;;i--) {
     if (true) continue;
     i--;
   }
+
+  // But do warn if the continue is in a nested loop.
+  for (;;i--) { // expected-note{{decremented here}}
+    for (int j = 0; j < 10; ++j) continue;
+    i--; // expected-warning{{decremented both}}
+  }
 }
 
 int f(int);
 void test9() {
   // Don't warn when variable is defined by the loop condition.
   for (int i = 0; int x = f(i); ++i) {}
 }