loplugin:unusedfields - look for fields that can be const, in comphelper

idea from tml.

Extend the unusedfields plugin to find fields that are only assigned in
the constructor.

Change-Id: I258d3581afbe651d53ce730c9ba27a4598cd9248
Reviewed-on: https://gerrit.libreoffice.org/57733
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 63 insertions, 0 deletions

diff --git a/compilerplugins/clang/unusedfields.cxx b/compilerplugins/clang/unusedfields.cxx
index 8dca2f549933..94ad63a6f3a3 100644
--- a/compilerplugins/clang/unusedfields.cxx
+++ b/compilerplugins/clang/unusedfields.cxx
@@ -70,6 +70,7 @@ static std::set<MyFieldInfo> touchedFromOutsideSet;
 static std::set<MyFieldInfo> touchedFromOutsideConstructorSet;
 static std::set<MyFieldInfo> readFromSet;
 static std::set<MyFieldInfo> writeToSet;
+static std::set<MyFieldInfo> writeToOutsideConstructorSet;
 static std::set<MyFieldInfo> definitionSet;
 
 /**
@@ -159,6 +160,7 @@ public:
 private:
     MyFieldInfo niceName(const FieldDecl*);
     void checkTouchedFromOutside(const FieldDecl* fieldDecl, const Expr* memberExpr);
+    void checkWriteFromOutsideConstructor(const FieldDecl* fieldDecl, const Expr* memberExpr);
     void checkWriteOnly(const FieldDecl* fieldDecl, const Expr* memberExpr);
     void checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberExpr);
     bool isSomeKindOfZero(const Expr* arg);
@@ -193,6 +195,8 @@ void UnusedFields::run()
             output += "read:\t" + s.parentClass + "\t" + s.fieldName + "\n";
         for (const MyFieldInfo & s : writeToSet)
             output += "write:\t" + s.parentClass + "\t" + s.fieldName + "\n";
+        for (const MyFieldInfo & s : writeToOutsideConstructorSet)
+            output += "write-outside-constructor:\t" + s.parentClass + "\t" + s.fieldName + "\n";
         for (const MyFieldInfo & s : definitionSet)
             output += "definition:\t" + s.access + "\t" + s.parentClass + "\t" + s.fieldName + "\t" + s.fieldType + "\t" + s.sourceLocation + "\n";
         std::ofstream myfile;
@@ -671,7 +675,11 @@ void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberE
         RecordDecl const * cxxRecordDecl1 = fieldDecl->getParent();
         // we don't care about writes to a field when inside the copy/move constructor/operator= for that field
         if (cxxRecordDecl1 && (cxxRecordDecl1 == insideMoveOrCopyOrCloneDeclParent))
+        {
+            // ... but they matter to tbe can-be-const analysis
+            checkWriteFromOutsideConstructor(fieldDecl, memberExpr);
             return;
+        }
     }
 
     // if we're inside a block that looks like
@@ -881,7 +889,10 @@ void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberE
 
     MyFieldInfo fieldInfo = niceName(fieldDecl);
     if (bPotentiallyWrittenTo)
+    {
         writeToSet.insert(fieldInfo);
+        checkWriteFromOutsideConstructor(fieldDecl, memberExpr);
+    }
 }
 
 bool UnusedFields::IsPassedByNonConst(const FieldDecl* fieldDecl, const Stmt * child, CallerWrapper callExpr,
@@ -1010,6 +1021,27 @@ void UnusedFields::checkTouchedFromOutside(const FieldDecl* fieldDecl, const Exp
     }
 }
 
+// For the const-field analysis.
+// Called when we have a write to a field, and we want to record that write only if it's writing from
+// outside the constructor.
+void UnusedFields::checkWriteFromOutsideConstructor(const FieldDecl* fieldDecl, const Expr* memberExpr) {
+    const FunctionDecl* memberExprParentFunction = getParentFunctionDecl(memberExpr);
+    bool doWrite = false;
+
+    if (!memberExprParentFunction)
+        // If we are not inside a function
+        doWrite = true;
+    else if (memberExprParentFunction->getParent() != fieldDecl->getParent())
+        // or we are inside a method from another class (than the one the field belongs to)
+        doWrite = true;
+    else if (!isa<CXXConstructorDecl>(memberExprParentFunction))
+        // or we are not inside constructor
+        doWrite = true;
+
+    if (doWrite)
+        writeToOutsideConstructorSet.insert(niceName(fieldDecl));
+}
+
 llvm::Optional<CalleeWrapper> UnusedFields::getCallee(CallExpr const * callExpr)
 {
     FunctionDecl const * functionDecl = callExpr->getDirectCallee();
diff --git a/compilerplugins/clang/unusedfields.py b/compilerplugins/clang/unusedfields.py
index 072284576a6a..a7910bfd9768 100755
--- a/compilerplugins/clang/unusedfields.py
+++ b/compilerplugins/clang/unusedfields.py
@@ -13,6 +13,7 @@ touchedFromOutsideSet = set()
 touchedFromOutsideConstructorSet = set()
 readFromSet = set()
 writeToSet = set()
+writeFromOutsideConstructorSet = set()
 sourceLocationSet = set()
 
 # clang does not always use exactly the same numbers in the type-parameter vars it generates
@@ -55,6 +56,8 @@ with io.open("workdir/loplugin.unusedfields.log", "rb", buffering=1024*1024) as
             readFromSet.add(parseFieldInfo(tokens))
         elif tokens[0] == "write:":
             writeToSet.add(parseFieldInfo(tokens))
+        elif tokens[0] == "write-outside-constructor:":
+            writeFromOutsideConstructorSet.add(parseFieldInfo(tokens))
         else:
             print( "unknown line: " + line)
 
@@ -223,6 +226,29 @@ for d in protectedAndPublicDefinitionSet:
 
     canBePrivateSet.add((clazz + " " + definitionToTypeMap[d], srcLoc))
 
+# Calculate can-be-const-field set
+canBeConstFieldSet = set()
+for d in definitionSet:
+    if d in writeFromOutsideConstructorSet:
+        continue
+    srcLoc = definitionToSourceLocationMap[d];
+    fieldType = definitionToTypeMap[d]
+    if fieldType.startswith("const "):
+        continue
+    if "std::unique_ptr" in fieldType:
+        continue
+    if "std::shared_ptr" in fieldType:
+        continue
+    if "Reference<" in fieldType:
+        continue
+    if "VclPtr<" in fieldType:
+        continue
+    if "osl::Mutex" in fieldType:
+        continue
+    if "::sfx2::sidebar::ControllerItem" in fieldType:
+        continue
+    canBeConstFieldSet.add((d[0] + " " + d[1] + " " + fieldType, srcLoc))
+
 
 # sort the results using a "natural order" so sequences like [item1,item2,item10] sort nicely
 def natural_sort_key(s, _nsre=re.compile('([0-9]+)')):
@@ -235,6 +261,7 @@ tmp2list = sorted(writeonlySet, key=lambda v: natural_sort_key(v[1]))
 tmp3list = sorted(canBePrivateSet, key=lambda v: natural_sort_key(v[1]))
 tmp4list = sorted(readonlySet, key=lambda v: natural_sort_key(v[1]))
 tmp5list = sorted(onlyUsedInConstructorSet, key=lambda v: natural_sort_key(v[1]))
+tmp6list = sorted(canBeConstFieldSet, key=lambda v: natural_sort_key(v[1]))
 
 # print out the results
 with open("compilerplugins/clang/unusedfields.untouched.results", "wt") as f:
@@ -258,5 +285,9 @@ with open("compilerplugins/clang/unusedfields.only-used-in-constructor.results",
     for t in tmp5list:
         f.write( t[1] + "\n" )
         f.write( "    " + t[0] + "\n" )
+with open("compilerplugins/clang/unusedfields.can-be-const.results", "wt") as f:
+    for t in tmp6list:
+        f.write( t[1] + "\n" )
+        f.write( "    " + t[0] + "\n" )