loplugin:unusedfields fix more false +

deal with fields assigned to local variables, and some general cleanup

Change-Id: I894c74a01e9e28935ecd84308c2e92b080afafc6

2 files changed, 53 insertions, 59 deletions

diff --git a/compilerplugins/clang/test/unusedfields.cxx b/compilerplugins/clang/test/unusedfields.cxx
index d24e6551c660..1d43c81ea177 100644
--- a/compilerplugins/clang/test/unusedfields.cxx
+++ b/compilerplugins/clang/test/unusedfields.cxx
@@ -7,7 +7,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-#include <rtl/ustring.hxx>
+#include <vector>
 
 struct Foo
 // expected-error@-1 {{read m_foo1 [loplugin:unusedfields]}}
@@ -19,25 +19,37 @@ struct Bar
 // expected-error@-1 {{read m_bar2 [loplugin:unusedfields]}}
 // expected-error@-2 {{read m_bar3 [loplugin:unusedfields]}}
 // expected-error@-3 {{read m_bar4 [loplugin:unusedfields]}}
-// expected-error@-4 {{read m_functionpointer [loplugin:unusedfields]}}
+// expected-error@-4 {{read m_bar5 [loplugin:unusedfields]}}
+// expected-error@-5 {{read m_bar6 [loplugin:unusedfields]}}
+// expected-error@-6 {{read m_barfunctionpointer [loplugin:unusedfields]}}
 {
-    int m_bar1;
-    int m_bar2 = 1;
+    int  m_bar1;
+    int  m_bar2 = 1;
     int* m_bar3;
-    int m_bar4;
-    void (*m_functionpointer)(int&);
+    int  m_bar4;
+    void (*m_barfunctionpointer)(int&);
+    int  m_bar5;
+    std::vector<int> m_bar6;
+
     //check that we see reads of fields when referred to via constructor initializer
     Bar(Foo const & foo) : m_bar1(foo.m_foo1) {}
 
-    int bar1() { return m_bar2; }
+    // check that we DONT see reads here
+    int bar2() { return m_bar2; }
 
-    // check that we see reads of fields when operated on via pointer de-ref
-    void bar2() { *m_bar3 = 2; }
+    // check that we DONT see reads here
+    void bar3()  { *m_bar3 = 2; }
+    void bar3b() { m_bar3 = nullptr; }
 
     // check that we see reads of field when passed to a function pointer
     // check that we see read of a field that is a function pointer
-    void bar3() { m_functionpointer(m_bar4); }
+    void bar4() { m_barfunctionpointer(m_bar4); }
+
+    // check that we see reads of a field when used in variable init
+    void bar5() { int x = m_bar5; (void) x; }
 
+    // check that we see reads of a field when used in ranged-for
+    void bar6() { for (auto i : m_bar6) { (void)i; } }
 };
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/unusedfields.cxx b/compilerplugins/clang/unusedfields.cxx
index 824efa5c41f7..d12cd19e275f 100644
--- a/compilerplugins/clang/unusedfields.cxx
+++ b/compilerplugins/clang/unusedfields.cxx
@@ -145,7 +145,7 @@ MyFieldInfo UnusedFields::niceName(const FieldDecl* fieldDecl)
     }
 
     aInfo.fieldName = fieldDecl->getNameAsString();
-    // sometimes the name (if it's anonymous thing) contains the full path of the build folder, which we don't need
+    // sometimes the name (if it's an anonymous thing) contains the full path of the build folder, which we don't need
     size_t idx = aInfo.fieldName.find(SRCDIR);
     if (idx != std::string::npos) {
         aInfo.fieldName = aInfo.fieldName.replace(idx, strlen(SRCDIR), "");
@@ -179,27 +179,6 @@ bool UnusedFields::VisitFieldDecl( const FieldDecl* fieldDecl )
         return true;
     }
 
-    QualType type = fieldDecl->getType();
-    // unwrap array types
-    while (type->isArrayType())
-        type = type->getAsArrayTypeUnsafe()->getElementType();
-/*
-    if( CXXRecordDecl* recordDecl = type->getAsCXXRecordDecl() )
-    {
-        bool warn_unused = recordDecl->hasAttr<WarnUnusedAttr>();
-        if( !warn_unused )
-        {
-            string n = recordDecl->getQualifiedNameAsString();
-            // Check some common non-LO types.
-            if( n == "std::string" || n == "std::basic_string"
-                || n == "std::list" || n == "std::__debug::list"
-                || n == "std::vector" || n == "std::__debug::vector" )
-                warn_unused = true;
-        }
-        if (!warn_unused)
-            return true;
-    }
-*/
     definitionSet.insert(niceName(fieldDecl));
     return true;
 }
@@ -245,23 +224,24 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
     // walk up the tree until we find something interesting
     bool bPotentiallyReadFrom = false;
     bool bDump = false;
-    do {
-        if (!parent) {
-            // check if we're inside a CXXCtorInitializer
+    do
+    {
+        if (!parent)
+        {
+            // check if we're inside a CXXCtorInitializer or a VarDecl
             auto parentsRange = compiler.getASTContext().getParents(*child);
             if ( parentsRange.begin() != parentsRange.end())
             {
                 const Decl* decl = parentsRange.begin()->get<Decl>();
-                if (decl && isa<CXXConstructorDecl>(decl))
+                if (decl && (isa<CXXConstructorDecl>(decl) || isa<VarDecl>(decl)))
                     bPotentiallyReadFrom = true;
             }
             if (!bPotentiallyReadFrom)
                 return true;
-            else
-                break;
+            break;
         }
         if (isa<CastExpr>(parent) || isa<MemberExpr>(parent) || isa<ParenExpr>(parent) || isa<ParenListExpr>(parent)
-             || isa<ExprWithCleanups>(parent))
+             || isa<ExprWithCleanups>(parent) || isa<ArrayInitLoopExpr>(parent))
         {
             child = parent;
             auto parentsRange = compiler.getASTContext().getParents(*parent);
@@ -283,20 +263,19 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
             auto parentsRange = compiler.getASTContext().getParents(*parent);
             parent = parentsRange.begin() == parentsRange.end() ? nullptr : parentsRange.begin()->get<Stmt>();
         }
-        else if (isa<CaseStmt>(parent))
+        else if (auto caseStmt = dyn_cast<CaseStmt>(parent))
         {
-            bPotentiallyReadFrom = dyn_cast<CaseStmt>(parent)->getLHS() == child
-                                  || dyn_cast<CaseStmt>(parent)->getRHS() == child;
+            bPotentiallyReadFrom = caseStmt->getLHS() == child || caseStmt->getRHS() == child;
             break;
         }
-        else if (isa<IfStmt>(parent))
+        else if (auto ifStmt = dyn_cast<IfStmt>(parent))
         {
-            bPotentiallyReadFrom = dyn_cast<IfStmt>(parent)->getCond() == child;
+            bPotentiallyReadFrom = ifStmt->getCond() == child;
             break;
         }
-        else if (isa<DoStmt>(parent))
+        else if (auto doStmt = dyn_cast<DoStmt>(parent))
         {
-            bPotentiallyReadFrom = dyn_cast<DoStmt>(parent)->getCond() == child;
+            bPotentiallyReadFrom = doStmt->getCond() == child;
             break;
         }
         else if (auto callExpr = dyn_cast<CallExpr>(parent))
@@ -312,7 +291,7 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
                     ;
                 else if (name == "clear" || name == "dispose" || name == "clearAndDispose" || name == "swap")
                     // we're abusing the write-only analysis here to look for fields which don't have anything useful
-                    // being done to them, so we're ignoring things like std::vector::clear, vector::swap,
+                    // being done to them, so we're ignoring things like std::vector::clear, std::vector::swap,
                     // and VclPtr::clearAndDispose
                     ;
                 else
@@ -325,16 +304,11 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
         else if (auto binaryOp = dyn_cast<BinaryOperator>(parent))
         {
             BinaryOperator::Opcode op = binaryOp->getOpcode();
-            // If the child is on the LHS and it is an assignment, we are obviously not reading from it,
-            // so walk up the tree.
-            if (binaryOp->getLHS() == child && op == BO_Assign) {
-                child = parent;
-                auto parentsRange = compiler.getASTContext().getParents(*parent);
-                parent = parentsRange.begin() == parentsRange.end() ? nullptr : parentsRange.begin()->get<Stmt>();
-            } else {
+            // If the child is on the LHS and it is an assignment, we are obviously not reading from it
+            if (!(binaryOp->getLHS() == child && op == BO_Assign)) {
                 bPotentiallyReadFrom = true;
-                break;
             }
+            break;
         }
         else if (isa<ReturnStmt>(parent)
                  || isa<CXXConstructExpr>(parent)
@@ -348,7 +322,7 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
                  || isa<InitListExpr>(parent)
                  || isa<CXXDependentScopeMemberExpr>(parent)
                  || isa<UnresolvedMemberExpr>(parent)
-                 || isa<MaterializeTemporaryExpr>(parent))  //???
+                 || isa<MaterializeTemporaryExpr>(parent))
         {
             bPotentiallyReadFrom = true;
             break;
@@ -364,12 +338,14 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
         {
             break;
         }
-        else {
+        else
+        {
             bPotentiallyReadFrom = true;
             bDump = true;
             break;
         }
     } while (true);
+
     if (bDump)
     {
         report(
@@ -377,12 +353,18 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
              "oh dear, what can the matter be?",
               memberExpr->getLocStart())
               << memberExpr->getSourceRange();
+        report(
+             DiagnosticsEngine::Note,
+             "parent over here",
+              parent->getLocStart())
+              << parent->getSourceRange();
         parent->dump();
+        memberExpr->dump();
     }
+
     if (bPotentiallyReadFrom)
-    {
         readFromSet.insert(fieldInfo);
-    }
+
     return true;
 }