-Warc-repeated-use-of-weak: allow single reads in loops from local variables.

Previously, the warning would erroneously fire on this:

for (Test *a in someArray)
  use(a.weakProp);

...because it looks like the same property is being accessed over and over.
However, clearly this is not the case. We now ignore loops like this for
local variables, but continue to warn if the base object is a parameter,
global variable, or instance variable, on the assumption that these are
not repeatedly usually assigned to within loops.

Additionally, do-while loops where the condition is 'false' are not really
loops at all; usually they're just used for semicolon-swallowing macros or
using "break" like "goto".

<rdar://problem/12578785&12578849>

llvm-svn: 166942

diff --git a/clang/include/clang/Sema/ScopeInfo.h b/clang/include/clang/Sema/ScopeInfo.h
index c49dee7..feda9c9 100644 (file)
--- a/clang/include/clang/Sema/ScopeInfo.h
+++ b/clang/include/clang/Sema/ScopeInfo.h
@@ -169,6 +169,7 @@ public:
     WeakObjectProfileTy(const DeclRefExpr *RE);
     WeakObjectProfileTy(const ObjCIvarRefExpr *RE);
 
+    const NamedDecl *getBase() const { return Base.getPointer(); }
     const NamedDecl *getProperty() const { return Property; }
 
     /// Returns true if the object base specifies a known object in memory,

diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index b039bc6..a20817f 100644 (file)
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -904,17 +904,24 @@ public:
 };
 }
 
-static bool isInLoop(const ParentMap &PM, const Stmt *S) {
+static bool isInLoop(const ASTContext &Ctx, 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;
+    case Stmt::DoStmtClass: {
+      const Expr *Cond = cast<DoStmt>(S)->getCond();
+      llvm::APSInt Val;
+      if (!Cond->EvaluateAsInt(Val, Ctx))
+        return true;
+      return Val.getBoolValue();
+    }
     default:
       break;
     }
@@ -932,6 +939,8 @@ static void diagnoseRepeatedUseOfWeak(Sema &S,
   typedef sema::FunctionScopeInfo::WeakObjectUseMap WeakObjectUseMap;
   typedef sema::FunctionScopeInfo::WeakUseVector WeakUseVector;
 
+  ASTContext &Ctx = S.getASTContext();
+
   const WeakObjectUseMap &WeakMap = CurFn->getWeakObjectUses();
 
   // Extract all weak objects that are referenced more than once.
@@ -952,16 +961,32 @@ static void diagnoseRepeatedUseOfWeak(Sema &S,
       continue;
 
     // If there was only one read, followed by any number of writes, and the
-    // read is not within a loop, don't warn.
+    // read is not within a loop, don't warn. Additionally, don't warn in a
+    // loop if the base object is a local variable -- local variables are often
+    // changed in loops.
     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()))
+      if (UI2 == UE) {
+        if (!isInLoop(Ctx, PM, UI->getUseExpr()))
           continue;
+
+        const WeakObjectProfileTy &Profile = I->first;
+        if (!Profile.isExactProfile())
+          continue;
+
+        const NamedDecl *Base = Profile.getBase();
+        if (!Base)
+          Base = Profile.getProperty();
+        assert(Base && "A profile always has a base or property.");
+
+        if (const VarDecl *BaseVar = dyn_cast<VarDecl>(Base))
+          if (BaseVar->hasLocalStorage() && !isa<ParmVarDecl>(Base))
+            continue;
+      }
     }
 
     UsesByStmt.push_back(StmtUsesPair(UI->getUseExpr(), I));

diff --git a/clang/test/SemaObjC/arc-repeated-weak.mm b/clang/test/SemaObjC/arc-repeated-weak.mm
index 1539a9b..e652bee 100644 (file)
--- a/clang/test/SemaObjC/arc-repeated-weak.mm
+++ b/clang/test/SemaObjC/arc-repeated-weak.mm
@@ -257,6 +257,38 @@ void readOnlyLoop(Test *a) {
   }
 }
 
+void readInIterationLoop() {
+  for (Test *a in get())
+    use(a.weakProp); // no-warning
+}
+
+void readDoubleLevelAccessInLoop() {
+  for (Test *a in get()) {
+    use(a.strongProp.weakProp); // no-warning
+  }
+}
+
+void readParameterInLoop(Test *a) {
+  for (id unused in get()) {
+    use(a.weakProp); // expected-warning{{weak property 'weakProp' is accessed multiple times in this function}}
+    (void)unused;
+  }
+}
+
+void readGlobalInLoop() {
+  static __weak id a;
+  for (id unused in get()) {
+    use(a); // expected-warning{{weak variable 'a' is accessed multiple times in this function}}
+    (void)unused;
+  }
+}
+
+void doWhileLoop(Test *a) {
+  do {
+    use(a.weakProp); // no-warning
+  } while(0);
+}
+
 
 @interface Test (Methods)
 @end
@@ -351,3 +383,4 @@ void doubleLevelAccessIvar(Test *a, Test *b) {
 
   use(a.strongProp.weakProp); // no-warning
 }
+