[Analysis] -Wunreachable-code shouldn't fire on the increment of a foreach loop

[sammccall]

Summary

The idea is that the code here isn't written, so doesn't indicate a bug.
Similar to code expanded from macros.

This means the warning no longer fires on this code:

for (auto C : collection) {
  process(C);
  return;
}
handleEmptyCollection();

Unclear whether this is more often a bug or not in practice, I think it's a
reasonable idiom in some cases.
Either way, if we want to warn on "loop that doesn't loop", I think it should be
a separate warning, and catch while(1) break;

[ilya-biryukov]

LGTM

lib/Analysis/ReachableCode.cpp

void DeadCodeScan::reportDeadCode(const CFGBlock *B,
 
   SourceRange SilenceableCondVal;
 
   if (UK == reachable_code::UK_Other) {
     // Check if the dead code is part of the "loop target" of
     // a for/for-range loop.  This is the block that contains
     // the increment code.
     if (const Stmt *LoopTarget = B->getLoopTarget()) {
+      // The increment on a foreach statement is not written.
+      if (isa<CXXForRangeStmt>(LoopTarget))
+        return;
+
       SourceLocation Loc = LoopTarget->getBeginLoc();
       SourceRange R1(Loc, Loc), R2;
 
       if (const ForStmt *FS = dyn_cast<ForStmt>(LoopTarget)) {
         const Expr *Inc = FS->getInc();
         Loc = Inc->getBeginLoc();
         R2 = Inc->getSourceRange();
       }

test/SemaCXX/unreachable-code.cpp

^{ return;
      bar();    // expected-warning {{will never be executed}}
   }();
   while (++j) {
     continue;
     bar();     // expected-warning {{will never be executed}}
   }
 }
 
+void test4() {
+  for (char c : "abc") // no-warning
+    break;
+}
+
 // PR 6130 - Don't warn about bogus unreachable code with throw's and
 // temporary objects.
 class PR6130 {
 public:
   PR6130();
   ~PR6130();
 };
 
 int pr6130(unsigned i) {
   switch(i) {
     case 0: return 1;
     case 1: return 2;
     default:
       throw PR6130(); // no-warning
   }
 }