-Warc-repeated-use-of-weak: Don't warn on a single read followed by writes.

This is a "safe" pattern, or at least one that cannot be helped by using
a strong local variable. However, if the single read is within a loop,
it should /always/ be treated as potentially dangerous.

<rdar://problem/12437490>

llvm-svn: 165719

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -27,6 +27,7 @@
 #include "clang/AST/StmtObjC.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
+#include "clang/AST/ParentMap.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Analysis/AnalysisContext.h"
@@ -903,10 +904,30 @@ public:
 };
 }
 
+static bool isInLoop(const ParentMap &PM, const Stmt *S) {
+  assert(S);
+
+  do {
+    switch (S->getStmtClass()) {
+    case Stmt::DoStmtClass:
+    case Stmt::ForStmtClass:
+    case Stmt::WhileStmtClass:
+    case Stmt::CXXForRangeStmtClass:
+    case Stmt::ObjCForCollectionStmtClass:
+      return true;
+    default:
+      break;
+    }
+  } while ((S = PM.getParent(S)));
+
+  return false;
+}
+
 
 static void diagnoseRepeatedUseOfWeak(Sema &S,
                                       const sema::FunctionScopeInfo *CurFn,
-                                      const Decl *D) {
+                                      const Decl *D,
+                                      const ParentMap &PM) {
   typedef sema::FunctionScopeInfo::WeakObjectProfileTy WeakObjectProfileTy;
   typedef sema::FunctionScopeInfo::WeakObjectUseMap WeakObjectUseMap;
   typedef sema::FunctionScopeInfo::WeakUseVector WeakUseVector;
@@ -918,8 +939,6 @@ static void diagnoseRepeatedUseOfWeak(Sema &S,
   for (WeakObjectUseMap::const_iterator I = WeakMap.begin(), E = WeakMap.end();
        I != E; ++I) {
     const WeakUseVector &Uses = I->second;
-    if (Uses.size() <= 1)
-      continue;
 
     // Find the first read of the weak object.
     WeakUseVector::const_iterator UI = Uses.begin(), UE = Uses.end();
@@ -932,6 +951,19 @@ static void diagnoseRepeatedUseOfWeak(Sema &S,
     if (UI == UE)
       continue;
 
+    // If there was only one read, followed by any number of writes, and the
+    // read is not within a loop, don't warn.
+    if (UI == Uses.begin()) {
+      WeakUseVector::const_iterator UI2 = UI;
+      for (++UI2; UI2 != UE; ++UI2)
+        if (UI2->isUnsafe())
+          break;
+
+      if (UI2 == UE)
+        if (!isInLoop(PM, UI->getUseExpr()))
+          continue;
+    }
+
     UsesByStmt.push_back(StmtUsesPair(UI->getUseExpr(), I));
   }
 
@@ -1519,7 +1551,7 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
   if (S.getLangOpts().ObjCARCWeak &&
       Diags.getDiagnosticLevel(diag::warn_arc_repeated_use_of_weak,
                                D->getLocStart()) != DiagnosticsEngine::Ignored)
-    diagnoseRepeatedUseOfWeak(S, fscope, D);
+    diagnoseRepeatedUseOfWeak(S, fscope, D, AC.getParentMap());
 
   // Collect statistics about the CFG if it was built.
   if (S.CollectStats && AC.isCFGBuilt()) {

clang/test/SemaObjC/arc-repeated-weak.mm

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-arc -fblocks -Wno-objc-root-class -Warc-repeated-use-of-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-arc -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
 
 @interface Test {
 @public
@@ -181,6 +181,70 @@ void assignToStrongWithCasts(Test *a) {
   }
 }
 
+void assignAfterRead(Test *a) {
+  // Special exception for a single read before any writes.
+  if (!a.weakProp) // no-warning
+    a.weakProp = get(); // no-warning
+}
+
+void readOnceWriteMany(Test *a) {
+  if (!a.weakProp) { // no-warning
+    a.weakProp = get(); // no-warning
+    a.weakProp = get(); // no-warning
+  }
+}
+
+void readOnceAfterWrite(Test *a) {
+  a.weakProp = get(); // expected-note{{also accessed here}}
+  if (!a.weakProp) { // expected-warning{{weak property 'weakProp' is accessed multiple times in this function}}
+    a.weakProp = get(); // expected-note{{also accessed here}}
+  }
+}
+
+void readOnceWriteManyLoops(Test *a, Test *b, Test *c, Test *d, Test *e) {
+  while (condition()) {
+    if (!a.weakProp) { // expected-warning{{weak property 'weakProp' is accessed multiple times in this function}}
+      a.weakProp = get(); // expected-note{{also accessed here}}
+      a.weakProp = get(); // expected-note{{also accessed here}}
+    }
+  }
+
+  do {
+    if (!b.weakProp) { // expected-warning{{weak property 'weakProp' is accessed multiple times in this function}}
+      b.weakProp = get(); // expected-note{{also accessed here}}
+      b.weakProp = get(); // expected-note{{also accessed here}}
+    }
+  } while (condition());
+
+  for (id x = get(); x; x = get()) {
+    if (!c.weakProp) { // expected-warning{{weak property 'weakProp' is accessed multiple times in this function}}
+      c.weakProp = get(); // expected-note{{also accessed here}}
+      c.weakProp = get(); // expected-note{{also accessed here}}
+    }
+  }
+
+  for (id x in get()) {
+    if (!d.weakProp) { // expected-warning{{weak property 'weakProp' is accessed multiple times in this function}}
+      d.weakProp = get(); // expected-note{{also accessed here}}
+      d.weakProp = get(); // expected-note{{also accessed here}}
+    }
+  }
+
+  int array[] = { 1, 2, 3 };
+  for (int i : array) {
+    if (!e.weakProp) { // expected-warning{{weak property 'weakProp' is accessed multiple times in this function}}
+      e.weakProp = get(); // expected-note{{also accessed here}}
+      e.weakProp = get(); // expected-note{{also accessed here}}
+    }
+  }
+}
+
+void readOnlyLoop(Test *a) {
+  while (condition()) {
+    use(a.weakProp); // expected-warning{{weak property 'weakProp' is accessed multiple times in this function}}
+  }
+}
+
 
 @interface Test (Methods)
 @end
@@ -248,11 +312,6 @@ public:
 
 // Most of these would require flow-sensitive analysis to silence correctly.
 
-void assignAfterRead(Test *a) {
-  if (!a.weakProp) // expected-warning{{weak property 'weakProp' is accessed multiple times}}
-    a.weakProp = get(); // expected-note{{also accessed here}}
-}
-
 void assignNil(Test *a) {
   if (condition())
     a.weakProp = nil; // expected-note{{also accessed here}}