unusedfields loplugin writeonly analysis improvements

(1) ignore reads inside copy/move constructors/operator=
(2) fix false+ when assigning to array field
(3) ignore reference ("&") fields

Change-Id: I69a1a1c567a0b28a783e605982e5150811b6cc4a

3 files changed, 67 insertions, 13 deletions

diff --git a/compilerplugins/clang/test/unusedfields.cxx b/compilerplugins/clang/test/unusedfields.cxx
index 1d43c81ea177..a66b7a6e7eca 100644
--- a/compilerplugins/clang/test/unusedfields.cxx
+++ b/compilerplugins/clang/test/unusedfields.cxx
@@ -17,29 +17,39 @@ struct Foo
 
 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_bar5 [loplugin:unusedfields]}}
-// expected-error@-5 {{read m_bar6 [loplugin:unusedfields]}}
-// expected-error@-6 {{read m_barfunctionpointer [loplugin:unusedfields]}}
+// expected-error@-2 {{read m_bar4 [loplugin:unusedfields]}}
+// expected-error@-3 {{read m_bar5 [loplugin:unusedfields]}}
+// expected-error@-4 {{read m_bar6 [loplugin:unusedfields]}}
+// expected-error@-5 {{read m_barfunctionpointer [loplugin:unusedfields]}}
 {
     int  m_bar1;
     int  m_bar2 = 1;
     int* m_bar3;
+    int* m_bar3b;
     int  m_bar4;
     void (*m_barfunctionpointer)(int&);
     int  m_bar5;
     std::vector<int> m_bar6;
+    int m_bar7[5];
 
-    //check that we see reads of fields when referred to via constructor initializer
+    // check that we see reads of fields like m_foo1 when referred to via constructor initializer
     Bar(Foo const & foo) : m_bar1(foo.m_foo1) {}
 
+    // check that we don't see reads when inside copy/move constructor
+    Bar(Bar const & other) { m_bar3 = other.m_bar3; }
+
+    // check that we don't see reads when inside copy/move assignment operator
+    Bar& operator=(Bar const & other) { m_bar3 = other.m_bar3; return *this; }
+
     // check that we DONT see reads here
     int bar2() { return m_bar2; }
 
     // check that we DONT see reads here
-    void bar3()  { *m_bar3 = 2; }
-    void bar3b() { m_bar3 = nullptr; }
+    void bar3()
+    {
+        m_bar3 = nullptr;
+        m_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
@@ -50,6 +60,8 @@ struct Bar
 
     // check that we see reads of a field when used in ranged-for
     void bar6() { for (auto i : m_bar6) { (void)i; } }
+
+    void bar7() { m_bar7[3] = 1; }
 };
 
 /* 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 1ba09f11e303..df13a7ce266f 100644
--- a/compilerplugins/clang/unusedfields.cxx
+++ b/compilerplugins/clang/unusedfields.cxx
@@ -80,9 +80,12 @@ public:
     bool VisitFieldDecl( const FieldDecl* );
     bool VisitMemberExpr( const MemberExpr* );
     bool VisitDeclRefExpr( const DeclRefExpr* );
+    bool TraverseCXXConstructorDecl( CXXConstructorDecl* );
+    bool TraverseCXXMethodDecl( CXXMethodDecl* );
 private:
     MyFieldInfo niceName(const FieldDecl*);
     void checkTouched(const FieldDecl* fieldDecl, const Expr* memberExpr);
+    CXXMethodDecl * insideMoveOrCopyDecl;
 };
 
 void UnusedFields::run()
@@ -189,11 +192,30 @@ static char easytolower(char in)
         return in-('Z'-'z');
     return in;
 }
+
 bool startswith(const std::string& rStr, const char* pSubStr)
 {
     return rStr.compare(0, strlen(pSubStr), pSubStr) == 0;
 }
 
+bool UnusedFields::TraverseCXXConstructorDecl(CXXConstructorDecl* cxxConstructorDecl)
+{
+    if (cxxConstructorDecl->isCopyOrMoveConstructor())
+        insideMoveOrCopyDecl = cxxConstructorDecl;
+    bool ret = RecursiveASTVisitor::TraverseCXXConstructorDecl(cxxConstructorDecl);
+    insideMoveOrCopyDecl = nullptr;
+    return ret;
+}
+
+bool UnusedFields::TraverseCXXMethodDecl(CXXMethodDecl* cxxMethodDecl)
+{
+    if (cxxMethodDecl->isCopyAssignmentOperator() || cxxMethodDecl->isMoveAssignmentOperator())
+        insideMoveOrCopyDecl = cxxMethodDecl;
+    bool ret = RecursiveASTVisitor::TraverseCXXMethodDecl(cxxMethodDecl);
+    insideMoveOrCopyDecl = nullptr;
+    return ret;
+}
+
 bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
 {
     const ValueDecl* decl = memberExpr->getMemberDecl();
@@ -218,6 +240,15 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
 
   // for the write-only analysis
 
+    // we don't care about reads from a field when inside that field parent class copy/move constructor/operator=
+    if (insideMoveOrCopyDecl)
+    {
+        auto cxxRecordDecl1 = dyn_cast<CXXRecordDecl>(fieldDecl->getDeclContext());
+        auto cxxRecordDecl2 = dyn_cast<CXXRecordDecl>(insideMoveOrCopyDecl->getDeclContext());
+        if (cxxRecordDecl1 && cxxRecordDecl1 == cxxRecordDecl2)
+            return true;
+    }
+
     auto parentsRange = compiler.getASTContext().getParents(*memberExpr);
     const Stmt* child = memberExpr;
     const Stmt* parent = parentsRange.begin() == parentsRange.end() ? nullptr : parentsRange.begin()->get<Stmt>();
@@ -253,7 +284,11 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
         else if (auto unaryOperator = dyn_cast<UnaryOperator>(parent))
         {
             UnaryOperator::Opcode op = unaryOperator->getOpcode();
-            if (op == UO_AddrOf || op == UO_Deref
+            if (memberExpr->getType()->isArrayType() && op == UO_Deref)
+            {
+                // ignore, deref'ing an array does not count as a read
+            }
+            else if (op == UO_AddrOf || op == UO_Deref
                 || op == UO_Plus || op == UO_Minus
                 || op == UO_Not || op == UO_LNot
                 || op == UO_PreInc || op == UO_PostInc
@@ -307,8 +342,12 @@ 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
-            if (!(binaryOp->getLHS() == child && op == BO_Assign)) {
+            // If the child is on the LHS and it is an assignment op, we are obviously not reading from it
+            const bool assignmentOp = op == BO_Assign || op == BO_MulAssign
+                || op == BO_DivAssign || op == BO_RemAssign || op == BO_AddAssign
+                || op == BO_SubAssign || op == BO_ShlAssign || op == BO_ShrAssign
+                || op == BO_AndAssign || op == BO_XorAssign || op == BO_OrAssign;
+            if (!(binaryOp->getLHS() == child && assignmentOp)) {
                 bPotentiallyReadFrom = true;
             }
             break;
@@ -317,7 +356,6 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
                  || isa<CXXConstructExpr>(parent)
                  || isa<ConditionalOperator>(parent)
                  || isa<SwitchStmt>(parent)
-                 || isa<ArraySubscriptExpr>(parent)
                  || isa<DeclStmt>(parent)
                  || isa<WhileStmt>(parent)
                  || isa<CXXNewExpr>(parent)
@@ -337,6 +375,7 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
                  || isa<LabelStmt>(parent)
                  || isa<CXXForRangeStmt>(parent)
                  || isa<CXXTypeidExpr>(parent)
+                 || isa<ArraySubscriptExpr>(parent)
                  || isa<DefaultStmt>(parent))
         {
             break;
@@ -414,7 +453,7 @@ void UnusedFields::checkTouched(const FieldDecl* fieldDecl, const Expr* memberEx
         if (memberExprParentFunction->getParent() == fieldDecl->getParent()) {
             touchedFromInsideSet.insert(fieldInfo);
         } else {
-           touchedFromOutsideSet.insert(fieldInfo);
+            touchedFromOutsideSet.insert(fieldInfo);
         }
     }
 }
diff --git a/compilerplugins/clang/unusedfields.py b/compilerplugins/clang/unusedfields.py
index 5d15b84ba528..0baf4c738cff 100755
--- a/compilerplugins/clang/unusedfields.py
+++ b/compilerplugins/clang/unusedfields.py
@@ -125,6 +125,9 @@ for d in definitionSet:
         continue
     if "::sfx2::sidebar::ControllerItem" in fieldType:
         continue
+    # ignore reference fields, because writing to them actually writes to another field somewhere else
+    if fieldType.endswith("&"):
+        continue
 
     writeonlySet.add((clazz + " " + definitionToTypeMap[d], srcLoc))