new loplugin:constfields

look for fields which are only assigned to in the constructor, so they
can be made const

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

compilerplugins/clang/constfields.py

+#!/usr/bin/python
+
+import sys
+import re
+import io
+
+definitionSet = set()
+definitionToSourceLocationMap = dict()
+definitionToTypeMap = dict()
+writeFromOutsideConstructorSet = set()
+
+# clang does not always use exactly the same numbers in the type-parameter vars it generates
+# so I need to substitute them to ensure we can match correctly.
+normalizeTypeParamsRegex = re.compile(r"type-parameter-\d+-\d+")
+def normalizeTypeParams( line ):
+    return normalizeTypeParamsRegex.sub("type-parameter-?-?", line)
+
+def parseFieldInfo( tokens ):
+    if len(tokens) == 3:
+        return (normalizeTypeParams(tokens[1]), tokens[2])
+    else:
+        return (normalizeTypeParams(tokens[1]), "")
+
+with io.open("workdir/loplugin.constfields.log", "rb", buffering=1024*1024) as txt:
+    for line in txt:
+        tokens = line.strip().split("\t")
+        if tokens[0] == "definition:":
+            access = tokens[1]
+            fieldInfo = (normalizeTypeParams(tokens[2]), tokens[3])
+            srcLoc = tokens[5]
+            # ignore external source code
+            if (srcLoc.startswith("external/")):
+                continue
+            # ignore build folder
+            if (srcLoc.startswith("workdir/")):
+                continue
+            definitionSet.add(fieldInfo)
+            definitionToTypeMap[fieldInfo] = tokens[4]
+            definitionToSourceLocationMap[fieldInfo] = tokens[5]
+        elif tokens[0] == "write-outside-constructor:":
+            writeFromOutsideConstructorSet.add(parseFieldInfo(tokens))
+        else:
+            print( "unknown line: " + line)
+
+# Invert the definitionToSourceLocationMap
+# If we see more than one method at the same sourceLocation, it's being autogenerated as part of a template
+# and we should just ignore it
+sourceLocationToDefinitionMap = {}
+for k, v in definitionToSourceLocationMap.iteritems():
+    sourceLocationToDefinitionMap[v] = sourceLocationToDefinitionMap.get(v, [])
+    sourceLocationToDefinitionMap[v].append(k)
+for k, definitions in sourceLocationToDefinitionMap.iteritems():
+    if len(definitions) > 1:
+        for d in definitions:
+            definitionSet.remove(d)
+
+# 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]+)')):
+    return [int(text) if text.isdigit() else text.lower()
+            for text in re.split(_nsre, s)]
+
+# sort results by name and line number
+tmp6list = sorted(canBeConstFieldSet, key=lambda v: natural_sort_key(v[1]))
+
+# print out the results
+with open("compilerplugins/clang/constfields.results", "wt") as f:
+    for t in tmp6list:
+        f.write( t[1] + "\n" )
+        f.write( "    " + t[0] + "\n" )
+
+

compilerplugins/clang/constfieldsrewrite.cxx

+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#if !defined _WIN32 //TODO, #include <sys/mman.h>
+
+#include <cassert>
+#include <string>
+#include <iostream>
+#include "plugin.hxx"
+#include "check.hxx"
+#include <sys/mman.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <assert.h>
+#include <cstring>
+
+/**
+  This is intended to be run as the second stage of the "constfields" clang plugin.
+*/
+
+namespace
+{
+class ConstFieldsRewrite : public RecursiveASTVisitor<ConstFieldsRewrite>,
+                           public loplugin::RewritePlugin
+{
+public:
+    explicit ConstFieldsRewrite(loplugin::InstantiationData const& data);
+    ~ConstFieldsRewrite();
+
+    virtual void run() override
+    {
+        if (rewriter)
+        {
+            TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+        }
+    }
+
+    bool VisitFieldDecl(const FieldDecl* var);
+
+private:
+    // I use a brute-force approach - mmap the results file and do a linear search on it
+    // It works surprisingly well, because the file is small enough to fit into L2 cache on modern CPU's
+    size_t mmapFilesize;
+    int mmapFD;
+    char* mmappedData;
+};
+
+size_t getFilesize(const char* filename)
+{
+    struct stat st;
+    stat(filename, &st);
+    return st.st_size;
+}
+
+ConstFieldsRewrite::ConstFieldsRewrite(loplugin::InstantiationData const& data)
+    : RewritePlugin(data)
+{
+    static const char sInputFile[] = SRCDIR "/compilerplugins/clang/constfields.results";
+    mmapFilesize = getFilesize(sInputFile);
+    //Open file
+    mmapFD = open(sInputFile, O_RDONLY, 0);
+    assert(mmapFD != -1);
+    //Execute mmap
+    mmappedData = static_cast<char*>(mmap(NULL, mmapFilesize, PROT_READ, MAP_PRIVATE, mmapFD, 0));
+    assert(mmappedData != NULL);
+}
+
+ConstFieldsRewrite::~ConstFieldsRewrite()
+{
+    //Cleanup
+    int rc = munmap(mmappedData, mmapFilesize);
+    assert(rc == 0);
+    close(mmapFD);
+}
+
+bool ConstFieldsRewrite::VisitFieldDecl(const FieldDecl* fieldDecl)
+{
+    if (ignoreLocation(fieldDecl))
+        return true;
+    // ignore stuff that forms part of the stable URE interface
+    if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc(
+            fieldDecl->getCanonicalDecl()->getLocation())))
+        return true;
+    // in case we've already processed this field
+    if (fieldDecl->getType().isConstQualified())
+        return true;
+    // in case we've already processed this field
+    if (fieldDecl->getType().isConstQualified())
+        return true;
+    // TODO rewriting T& is a bit trickier
+    if (loplugin::TypeCheck(fieldDecl->getType()).LvalueReference())
+        return true;
+
+    const RecordDecl* recordDecl = fieldDecl->getParent();
+    std::string parentClassName;
+    if (const CXXRecordDecl* cxxRecordDecl = dyn_cast<CXXRecordDecl>(recordDecl))
+    {
+        if (cxxRecordDecl->getTemplateInstantiationPattern())
+            cxxRecordDecl = cxxRecordDecl->getTemplateInstantiationPattern();
+        parentClassName = cxxRecordDecl->getQualifiedNameAsString();
+    }
+    else
+    {
+        parentClassName = recordDecl->getQualifiedNameAsString();
+    }
+    // the extra spaces match the formatting in the results file, and help avoid false+
+    std::string aNiceName = "    " + parentClassName + " " + fieldDecl->getNameAsString() + " "
+                            + fieldDecl->getType().getAsString() + "\n";
+
+    // search mmap'ed file for field
+    const char* aNiceNameStr = aNiceName.c_str();
+    char* found = std::search(mmappedData, mmappedData + mmapFilesize, aNiceNameStr,
+                              aNiceNameStr + aNiceName.size());
+    if (!(found < mmappedData + mmapFilesize))
+        return true;
+
+    auto endLoc = fieldDecl->getTypeSourceInfo()->getTypeLoc().getEndLoc();
+    endLoc = endLoc.getLocWithOffset(
+        Lexer::MeasureTokenLength(endLoc, compiler.getSourceManager(), compiler.getLangOpts()));
+
+    if (!insertText(endLoc, " const"))
+    {
+        report(DiagnosticsEngine::Warning, "Could not mark field as const",
+               compat::getBeginLoc(fieldDecl))
+            << fieldDecl->getSourceRange();
+    }
+    return true;
+}
+
+loplugin::Plugin::Registration<ConstFieldsRewrite> X("constfieldsrewrite", false);
+}
+
+#endif
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */

compilerplugins/clang/test/constfields.cxx

+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <vector>
+#include <ostream>
+#include <com/sun/star/uno/Any.hxx>
+
+struct Test0
+{
+    void method1() {}
+};
+
+// checking for calling non-const method
+struct Test1
+// expected-error@-1 {{notconst m_field1 [loplugin:constfields]}}
+{
+    Test0* m_field1;
+
+    void method1()
+    {
+        if (m_field1)
+            m_field1->method1();
+    }
+};
+
+// checking for assigning to field
+struct Test2
+// expected-error@-1 {{notconst m_field1 [loplugin:constfields]}}
+{
+    Test0* m_field1;
+
+    Test2()
+        : m_field1(nullptr)
+    {
+    }
+
+    void method1() { m_field1 = nullptr; }
+};
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */

compilerplugins/clang/unusedfields.cxx

@@ -70,7 +70,6 @@ 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;
 
 /**
@@ -160,7 +159,6 @@ 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);
@@ -195,8 +193,6 @@ 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;
@@ -676,8 +672,6 @@ void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberE
         // 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;
         }
     }
@@ -891,7 +885,6 @@ void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberE
     if (bPotentiallyWrittenTo)
     {
         writeToSet.insert(fieldInfo);
-        checkWriteFromOutsideConstructor(fieldDecl, memberExpr);
     }
 }
 
@@ -1021,27 +1014,6 @@ 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();

compilerplugins/clang/unusedfields.py

@@ -13,7 +13,6 @@ 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
@@ -56,8 +55,6 @@ 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)
 
@@ -226,30 +223,6 @@ 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]+)')):
     return [int(text) if text.isdigit() else text.lower()
@@ -261,7 +234,6 @@ 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:
@@ -285,9 +257,5 @@ 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" )
 
 

solenv/CompilerTest_compilerplugins_clang.mk

@@ -14,6 +14,7 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
     compilerplugins/clang/test/blockblock \
     compilerplugins/clang/test/casttovoid \
     compilerplugins/clang/test/commaoperator \
+    compilerplugins/clang/test/constfields \
     compilerplugins/clang/test/constparams \
     compilerplugins/clang/test/conststringfield \
     compilerplugins/clang/test/convertlong \