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>

7 files changed, 877 insertions, 60 deletions

diff --git a/compilerplugins/clang/constfields.cxx b/compilerplugins/clang/constfields.cxx
new file mode 100644
index 000000000000..6613c836b892
--- /dev/null
+++ b/compilerplugins/clang/constfields.cxx
@@ -0,0 +1,593 @@
+/* -*- 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/file.h>
+
+#include <cassert>
+#include <string>
+#include <iostream>
+#include <fstream>
+#include <unordered_set>
+#include <vector>
+#include <algorithm>
+#include <sys/file.h>
+#include <unistd.h>
+#include "plugin.hxx"
+#include "compat.hxx"
+#include "check.hxx"
+
+/**
+Look for fields that are only assigned to in the constructor using field-init, and can therefore be const.
+
+The process goes something like this:
+  $ make check
+  $ make FORCE_COMPILE_ALL=1 COMPILER_PLUGIN_TOOL='constfields' check
+  $ ./compilerplugins/clang/constfields.py
+
+and then
+  $ for dir in *; do make $dir FORCE_COMPILE_ALL=1 UPDATE_FILES=$dir COMPILER_PLUGIN_TOOL='constfieldsrewrite' $dir; done
+to auto-remove the method declarations
+
+*/
+
+namespace
+{
+struct MyFieldInfo
+{
+    const RecordDecl* parentRecord;
+    std::string parentClass;
+    std::string fieldName;
+    std::string fieldType;
+    std::string sourceLocation;
+    std::string access;
+};
+bool operator<(const MyFieldInfo& lhs, const MyFieldInfo& rhs)
+{
+    return std::tie(lhs.parentClass, lhs.fieldName) < std::tie(rhs.parentClass, rhs.fieldName);
+}
+
+// try to limit the voluminous output a little
+static std::set<MyFieldInfo> cannotBeConstSet;
+static std::set<MyFieldInfo> definitionSet;
+
+/**
+ * Wrap the different kinds of callable and callee objects in the clang AST so I can define methods that handle everything.
+ */
+class CallerWrapper
+{
+    const CallExpr* m_callExpr;
+    const CXXConstructExpr* m_cxxConstructExpr;
+
+public:
+    CallerWrapper(const CallExpr* callExpr)
+        : m_callExpr(callExpr)
+        , m_cxxConstructExpr(nullptr)
+    {
+    }
+    CallerWrapper(const CXXConstructExpr* cxxConstructExpr)
+        : m_callExpr(nullptr)
+        , m_cxxConstructExpr(cxxConstructExpr)
+    {
+    }
+    unsigned getNumArgs() const
+    {
+        return m_callExpr ? m_callExpr->getNumArgs() : m_cxxConstructExpr->getNumArgs();
+    }
+    const Expr* getArg(unsigned i) const
+    {
+        return m_callExpr ? m_callExpr->getArg(i) : m_cxxConstructExpr->getArg(i);
+    }
+};
+class CalleeWrapper
+{
+    const FunctionDecl* m_calleeFunctionDecl = nullptr;
+    const CXXConstructorDecl* m_cxxConstructorDecl = nullptr;
+    const FunctionProtoType* m_functionPrototype = nullptr;
+
+public:
+    explicit CalleeWrapper(const FunctionDecl* calleeFunctionDecl)
+        : m_calleeFunctionDecl(calleeFunctionDecl)
+    {
+    }
+    explicit CalleeWrapper(const CXXConstructExpr* cxxConstructExpr)
+        : m_cxxConstructorDecl(cxxConstructExpr->getConstructor())
+    {
+    }
+    explicit CalleeWrapper(const FunctionProtoType* functionPrototype)
+        : m_functionPrototype(functionPrototype)
+    {
+    }
+    unsigned getNumParams() const
+    {
+        if (m_calleeFunctionDecl)
+            return m_calleeFunctionDecl->getNumParams();
+        else if (m_cxxConstructorDecl)
+            return m_cxxConstructorDecl->getNumParams();
+        else if (m_functionPrototype->param_type_begin() == m_functionPrototype->param_type_end())
+            // FunctionProtoType will assert if we call getParamTypes() and it has no params
+            return 0;
+        else
+            return m_functionPrototype->getParamTypes().size();
+    }
+    const QualType getParamType(unsigned i) const
+    {
+        if (m_calleeFunctionDecl)
+            return m_calleeFunctionDecl->getParamDecl(i)->getType();
+        else if (m_cxxConstructorDecl)
+            return m_cxxConstructorDecl->getParamDecl(i)->getType();
+        else
+            return m_functionPrototype->getParamTypes()[i];
+    }
+    std::string getNameAsString() const
+    {
+        if (m_calleeFunctionDecl)
+            return m_calleeFunctionDecl->getNameAsString();
+        else if (m_cxxConstructorDecl)
+            return m_cxxConstructorDecl->getNameAsString();
+        else
+            return "";
+    }
+    CXXMethodDecl const* getAsCXXMethodDecl() const
+    {
+        if (m_calleeFunctionDecl)
+            return dyn_cast<CXXMethodDecl>(m_calleeFunctionDecl);
+        return nullptr;
+    }
+};
+
+class ConstFields : public RecursiveASTVisitor<ConstFields>, public loplugin::Plugin
+{
+public:
+    explicit ConstFields(loplugin::InstantiationData const& data)
+        : Plugin(data)
+    {
+    }
+
+    virtual void run() override;
+
+    bool shouldVisitTemplateInstantiations() const { return true; }
+    bool shouldVisitImplicitCode() const { return true; }
+
+    bool VisitFieldDecl(const FieldDecl*);
+    bool VisitMemberExpr(const MemberExpr*);
+    bool TraverseCXXConstructorDecl(CXXConstructorDecl*);
+    bool TraverseCXXMethodDecl(CXXMethodDecl*);
+    bool TraverseFunctionDecl(FunctionDecl*);
+    bool TraverseIfStmt(IfStmt*);
+
+private:
+    MyFieldInfo niceName(const FieldDecl*);
+    void check(const FieldDecl* fieldDecl, const Expr* memberExpr);
+    bool isSomeKindOfZero(const Expr* arg);
+    bool IsPassedByNonConst(const FieldDecl* fieldDecl, const Stmt* child, CallerWrapper callExpr,
+                            CalleeWrapper calleeFunctionDecl);
+    llvm::Optional<CalleeWrapper> getCallee(CallExpr const*);
+
+    RecordDecl* insideMoveOrCopyDeclParent = nullptr;
+    // For reasons I do not understand, parentFunctionDecl() is not reliable, so
+    // we store the parent function on the way down the AST.
+    FunctionDecl* insideFunctionDecl = nullptr;
+    std::vector<FieldDecl const*> insideConditionalCheckOfMemberSet;
+};
+
+void ConstFields::run()
+{
+    TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+
+    if (!isUnitTestMode())
+    {
+        // dump all our output in one write call - this is to try and limit IO "crosstalk" between multiple processes
+        // writing to the same logfile
+        std::string output;
+        for (const MyFieldInfo& s : cannotBeConstSet)
+            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;
+        myfile.open(WORKDIR "/loplugin.constfields.log", std::ios::app | std::ios::out);
+        myfile << output;
+        myfile.close();
+    }
+    else
+    {
+        for (const MyFieldInfo& s : cannotBeConstSet)
+            report(DiagnosticsEngine::Warning, "notconst %0", compat::getBeginLoc(s.parentRecord))
+                << s.fieldName;
+    }
+}
+
+MyFieldInfo ConstFields::niceName(const FieldDecl* fieldDecl)
+{
+    MyFieldInfo aInfo;
+
+    const RecordDecl* recordDecl = fieldDecl->getParent();
+
+    if (const CXXRecordDecl* cxxRecordDecl = dyn_cast<CXXRecordDecl>(recordDecl))
+    {
+        if (cxxRecordDecl->getTemplateInstantiationPattern())
+            cxxRecordDecl = cxxRecordDecl->getTemplateInstantiationPattern();
+        aInfo.parentRecord = cxxRecordDecl;
+        aInfo.parentClass = cxxRecordDecl->getQualifiedNameAsString();
+    }
+    else
+    {
+        aInfo.parentRecord = recordDecl;
+        aInfo.parentClass = recordDecl->getQualifiedNameAsString();
+    }
+
+    aInfo.fieldName = fieldDecl->getNameAsString();
+    // 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), "");
+    }
+    aInfo.fieldType = fieldDecl->getType().getAsString();
+
+    SourceLocation expansionLoc
+        = compiler.getSourceManager().getExpansionLoc(fieldDecl->getLocation());
+    StringRef name = compiler.getSourceManager().getFilename(expansionLoc);
+    aInfo.sourceLocation
+        = std::string(name.substr(strlen(SRCDIR) + 1)) + ":"
+          + std::to_string(compiler.getSourceManager().getSpellingLineNumber(expansionLoc));
+    loplugin::normalizeDotDotInFilePath(aInfo.sourceLocation);
+
+    switch (fieldDecl->getAccess())
+    {
+        case AS_public:
+            aInfo.access = "public";
+            break;
+        case AS_private:
+            aInfo.access = "private";
+            break;
+        case AS_protected:
+            aInfo.access = "protected";
+            break;
+        default:
+            aInfo.access = "unknown";
+            break;
+    }
+
+    return aInfo;
+}
+
+bool ConstFields::VisitFieldDecl(const FieldDecl* fieldDecl)
+{
+    fieldDecl = fieldDecl->getCanonicalDecl();
+    if (ignoreLocation(fieldDecl))
+    {
+        return true;
+    }
+    // ignore stuff that forms part of the stable URE interface
+    if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocation())))
+    {
+        return true;
+    }
+    definitionSet.insert(niceName(fieldDecl));
+    return true;
+}
+
+bool ConstFields::TraverseCXXConstructorDecl(CXXConstructorDecl* cxxConstructorDecl)
+{
+    auto copy = insideMoveOrCopyDeclParent;
+    if (!ignoreLocation(cxxConstructorDecl) && cxxConstructorDecl->isThisDeclarationADefinition())
+    {
+        if (cxxConstructorDecl->isCopyOrMoveConstructor())
+            insideMoveOrCopyDeclParent = cxxConstructorDecl->getParent();
+    }
+    bool ret = RecursiveASTVisitor::TraverseCXXConstructorDecl(cxxConstructorDecl);
+    insideMoveOrCopyDeclParent = copy;
+    return ret;
+}
+
+bool ConstFields::TraverseCXXMethodDecl(CXXMethodDecl* cxxMethodDecl)
+{
+    auto copy1 = insideMoveOrCopyDeclParent;
+    auto copy2 = insideFunctionDecl;
+    if (!ignoreLocation(cxxMethodDecl) && cxxMethodDecl->isThisDeclarationADefinition())
+    {
+        if (cxxMethodDecl->isCopyAssignmentOperator() || cxxMethodDecl->isMoveAssignmentOperator())
+            insideMoveOrCopyDeclParent = cxxMethodDecl->getParent();
+    }
+    insideFunctionDecl = cxxMethodDecl;
+    bool ret = RecursiveASTVisitor::TraverseCXXMethodDecl(cxxMethodDecl);
+    insideMoveOrCopyDeclParent = copy1;
+    insideFunctionDecl = copy2;
+    return ret;
+}
+
+bool ConstFields::TraverseFunctionDecl(FunctionDecl* functionDecl)
+{
+    auto copy2 = insideFunctionDecl;
+    insideFunctionDecl = functionDecl;
+    bool ret = RecursiveASTVisitor::TraverseFunctionDecl(functionDecl);
+    insideFunctionDecl = copy2;
+    return ret;
+}
+
+bool ConstFields::TraverseIfStmt(IfStmt* ifStmt)
+{
+    FieldDecl const* memberFieldDecl = nullptr;
+    Expr const* cond = ifStmt->getCond()->IgnoreParenImpCasts();
+    if (auto memberExpr = dyn_cast<MemberExpr>(cond))
+    {
+        if ((memberFieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl())))
+            insideConditionalCheckOfMemberSet.push_back(memberFieldDecl);
+    }
+    bool ret = RecursiveASTVisitor::TraverseIfStmt(ifStmt);
+    if (memberFieldDecl)
+        insideConditionalCheckOfMemberSet.pop_back();
+    return ret;
+}
+
+bool ConstFields::VisitMemberExpr(const MemberExpr* memberExpr)
+{
+    const ValueDecl* decl = memberExpr->getMemberDecl();
+    const FieldDecl* fieldDecl = dyn_cast<FieldDecl>(decl);
+    if (!fieldDecl)
+    {
+        return true;
+    }
+    fieldDecl = fieldDecl->getCanonicalDecl();
+    if (ignoreLocation(fieldDecl))
+    {
+        return true;
+    }
+    // ignore stuff that forms part of the stable URE interface
+    if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocation())))
+    {
+        return true;
+    }
+
+    check(fieldDecl, memberExpr);
+
+    return true;
+}
+
+void ConstFields::check(const FieldDecl* fieldDecl, const Expr* memberExpr)
+{
+    auto parentsRange = compiler.getASTContext().getParents(*memberExpr);
+    const Stmt* child = memberExpr;
+    const Stmt* parent
+        = parentsRange.begin() == parentsRange.end() ? nullptr : parentsRange.begin()->get<Stmt>();
+    // walk up the tree until we find something interesting
+    bool bCannotBeConst = false;
+    bool bDump = false;
+    auto walkUp = [&]() {
+        child = parent;
+        auto parentsRange = compiler.getASTContext().getParents(*parent);
+        parent = parentsRange.begin() == parentsRange.end() ? nullptr
+                                                            : parentsRange.begin()->get<Stmt>();
+    };
+    do
+    {
+        if (!parent)
+        {
+            // check if we have an expression like
+            //    int& r = m_field;
+            auto parentsRange = compiler.getASTContext().getParents(*child);
+            if (parentsRange.begin() != parentsRange.end())
+            {
+                auto varDecl = dyn_cast_or_null<VarDecl>(parentsRange.begin()->get<Decl>());
+                // The isImplicit() call is to avoid triggering when we see the vardecl which is part of a for-range statement,
+                // which is of type 'T&&' and also an l-value-ref ?
+                if (varDecl && !varDecl->isImplicit()
+                    && loplugin::TypeCheck(varDecl->getType()).LvalueReference().NonConst())
+                {
+                    bCannotBeConst = true;
+                }
+            }
+            break;
+        }
+        if (isa<CXXReinterpretCastExpr>(parent))
+        {
+            // once we see one of these, there is not much useful we can know
+            bCannotBeConst = true;
+            break;
+        }
+        else if (isa<CastExpr>(parent) || isa<MemberExpr>(parent) || isa<ParenExpr>(parent)
+                 || isa<ParenListExpr>(parent)
+#if CLANG_VERSION >= 40000
+                 || isa<ArrayInitLoopExpr>(parent)
+#endif
+                 || isa<ExprWithCleanups>(parent))
+        {
+            walkUp();
+        }
+        else if (auto unaryOperator = dyn_cast<UnaryOperator>(parent))
+        {
+            UnaryOperator::Opcode op = unaryOperator->getOpcode();
+            if (op == UO_AddrOf || op == UO_PostInc || op == UO_PostDec || op == UO_PreInc
+                || op == UO_PreDec)
+            {
+                bCannotBeConst = true;
+            }
+            walkUp();
+        }
+        else if (auto operatorCallExpr = dyn_cast<CXXOperatorCallExpr>(parent))
+        {
+            auto callee = getCallee(operatorCallExpr);
+            if (callee)
+            {
+                // if calling a non-const operator on the field
+                auto calleeMethodDecl = callee->getAsCXXMethodDecl();
+                if (calleeMethodDecl && operatorCallExpr->getArg(0) == child
+                    && !calleeMethodDecl->isConst())
+                {
+                    bCannotBeConst = true;
+                }
+                else if (IsPassedByNonConst(fieldDecl, child, operatorCallExpr, *callee))
+                {
+                    bCannotBeConst = true;
+                }
+            }
+            else
+                bCannotBeConst = true; // conservative, could improve
+            walkUp();
+        }
+        else if (auto cxxMemberCallExpr = dyn_cast<CXXMemberCallExpr>(parent))
+        {
+            const CXXMethodDecl* calleeMethodDecl = cxxMemberCallExpr->getMethodDecl();
+            if (calleeMethodDecl)
+            {
+                // if calling a non-const method on the field
+                const Expr* tmp = dyn_cast<Expr>(child);
+                if (tmp->isBoundMemberFunction(compiler.getASTContext()))
+                {
+                    tmp = dyn_cast<MemberExpr>(tmp)->getBase();
+                }
+                if (cxxMemberCallExpr->getImplicitObjectArgument() == tmp
+                    && !calleeMethodDecl->isConst())
+                {
+                    bCannotBeConst = true;
+                    break;
+                }
+                if (IsPassedByNonConst(fieldDecl, child, cxxMemberCallExpr,
+                                       CalleeWrapper(calleeMethodDecl)))
+                    bCannotBeConst = true;
+            }
+            else
+                bCannotBeConst = true; // can happen in templates
+            walkUp();
+        }
+        else if (auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(parent))
+        {
+            if (IsPassedByNonConst(fieldDecl, child, cxxConstructExpr,
+                                   CalleeWrapper(cxxConstructExpr)))
+                bCannotBeConst = true;
+            walkUp();
+        }
+        else if (auto callExpr = dyn_cast<CallExpr>(parent))
+        {
+            auto callee = getCallee(callExpr);
+            if (callee)
+            {
+                if (IsPassedByNonConst(fieldDecl, child, callExpr, *callee))
+                    bCannotBeConst = true;
+            }
+            else
+                bCannotBeConst = true; // conservative, could improve
+            walkUp();
+        }
+        else if (auto binaryOp = dyn_cast<BinaryOperator>(parent))
+        {
+            BinaryOperator::Opcode op = binaryOp->getOpcode();
+            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 (assignmentOp)
+            {
+                if (binaryOp->getLHS() == child)
+                    bCannotBeConst = true;
+                else if (loplugin::TypeCheck(binaryOp->getLHS()->getType())
+                             .LvalueReference()
+                             .NonConst())
+                    // if the LHS is a non-const reference, we could write to the field later on
+                    bCannotBeConst = true;
+            }
+            walkUp();
+        }
+        else if (isa<ReturnStmt>(parent))
+        {
+            if (insideFunctionDecl)
+            {
+                auto tc = loplugin::TypeCheck(insideFunctionDecl->getReturnType());
+                if (tc.LvalueReference().NonConst())
+                    bCannotBeConst = true;
+            }
+            break;
+        }
+        else if (isa<SwitchStmt>(parent) || isa<WhileStmt>(parent) || isa<ForStmt>(parent)
+                 || isa<IfStmt>(parent) || isa<DoStmt>(parent) || isa<CXXForRangeStmt>(parent)
+                 || isa<DefaultStmt>(parent))
+        {
+            break;
+        }
+        else
+        {
+            walkUp();
+        }
+    } while (true);
+
+    if (bDump)
+    {
+        report(DiagnosticsEngine::Warning, "oh dear, what can the matter be? writtenTo=%0",
+               compat::getBeginLoc(memberExpr))
+            << bCannotBeConst << memberExpr->getSourceRange();
+        if (parent)
+        {
+            report(DiagnosticsEngine::Note, "parent over here", compat::getBeginLoc(parent))
+                << parent->getSourceRange();
+            parent->dump();
+        }
+        memberExpr->dump();
+        fieldDecl->getType()->dump();
+    }
+
+    if (bCannotBeConst)
+    {
+        cannotBeConstSet.insert(niceName(fieldDecl));
+    }
+}
+
+bool ConstFields::IsPassedByNonConst(const FieldDecl* fieldDecl, const Stmt* child,
+                                     CallerWrapper callExpr, CalleeWrapper calleeFunctionDecl)
+{
+    unsigned len = std::min(callExpr.getNumArgs(), calleeFunctionDecl.getNumParams());
+    // if it's an array, passing it by value to a method typically means the
+    // callee takes a pointer and can modify the array
+    if (fieldDecl->getType()->isConstantArrayType())
+    {
+        for (unsigned i = 0; i < len; ++i)
+            if (callExpr.getArg(i) == child)
+                if (loplugin::TypeCheck(calleeFunctionDecl.getParamType(i)).Pointer().NonConst())
+                    return true;
+    }
+    else
+    {
+        for (unsigned i = 0; i < len; ++i)
+            if (callExpr.getArg(i) == child)
+                if (loplugin::TypeCheck(calleeFunctionDecl.getParamType(i))
+                        .LvalueReference()
+                        .NonConst())
+                    return true;
+    }
+    return false;
+}
+
+llvm::Optional<CalleeWrapper> ConstFields::getCallee(CallExpr const* callExpr)
+{
+    FunctionDecl const* functionDecl = callExpr->getDirectCallee();
+    if (functionDecl)
+        return CalleeWrapper(functionDecl);
+
+    // Extract the functionprototype from a type
+    clang::Type const* calleeType = callExpr->getCallee()->getType().getTypePtr();
+    if (auto pointerType = calleeType->getUnqualifiedDesugaredType()->getAs<clang::PointerType>())
+    {
+        if (auto prototype = pointerType->getPointeeType()
+                                 ->getUnqualifiedDesugaredType()
+                                 ->getAs<FunctionProtoType>())
+        {
+            return CalleeWrapper(prototype);
+        }
+    }
+
+    return llvm::Optional<CalleeWrapper>();
+}
+
+loplugin::Plugin::Registration<ConstFields> X("constfields", false);
+}
+
+#endif
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/compilerplugins/clang/constfields.py b/compilerplugins/clang/constfields.py
new file mode 100755
index 000000000000..9ce5e2747fda
--- /dev/null
+++ b/compilerplugins/clang/constfields.py
@@ -0,0 +1,95 @@
+#!/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" )
+
+
diff --git a/compilerplugins/clang/constfieldsrewrite.cxx b/compilerplugins/clang/constfieldsrewrite.cxx
new file mode 100644
index 000000000000..cff211665ddf
--- /dev/null
+++ b/compilerplugins/clang/constfieldsrewrite.cxx
@@ -0,0 +1,143 @@
+/* -*- 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: */
diff --git a/compilerplugins/clang/test/constfields.cxx b/compilerplugins/clang/test/constfields.cxx
new file mode 100644
index 000000000000..c045396d5432
--- /dev/null
+++ b/compilerplugins/clang/test/constfields.cxx
@@ -0,0 +1,45 @@
+/* -*- 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: */
diff --git a/compilerplugins/clang/unusedfields.cxx b/compilerplugins/clang/unusedfields.cxx
index ac8a4d1aabf2..5ba6ac3c1cd6 100644
--- a/compilerplugins/clang/unusedfields.cxx
+++ b/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();
diff --git a/compilerplugins/clang/unusedfields.py b/compilerplugins/clang/unusedfields.py
index a7910bfd9768..dcb37a72017f 100755
--- a/compilerplugins/clang/unusedfields.py
+++ b/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" )
 
 
diff --git a/solenv/CompilerTest_compilerplugins_clang.mk b/solenv/CompilerTest_compilerplugins_clang.mk
index 6aacf7547c60..ad9b1e3f5192 100644
--- a/solenv/CompilerTest_compilerplugins_clang.mk
+++ b/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 \