loplugin:passstuffbyref ignore params that are assigned to

makes writing nice code awkward sometimes.

Also split plugin into two different plugins, the logic was getting
tangled up.

Change-Id: I232e314d29c766c160c29373988dc37a466505be

compilerplugins/clang/passparamsbyref.cxx

@@ -0,0 +1,195 @@
+/* -*- 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/.
+ */
+
+#include <string>
+#include <unordered_set>
+
+#include "check.hxx"
+#include "compat.hxx"
+#include "plugin.hxx"
+
+// Find places where parameters are passed by value
+// It's not very efficient, because we generally end up copying it twice - once into the parameter and
+// again into the destination.
+// They should rather be passed by reference.
+//
+// Generally recommending lambda capture by-ref rather than by-copy is even more
+// problematic than with function parameters, as a lambda instance can easily
+// outlive a referenced variable. So once lambdas start to get used in more
+// sophisticated ways than passing them into standard algorithms, this plugin's
+// advice, at least for explicit captures, will need to be revisited.
+
+namespace {
+
+class PassParamsByRef:
+    public RecursiveASTVisitor<PassParamsByRef>, public loplugin::Plugin
+{
+public:
+    explicit PassParamsByRef(InstantiationData const & data): Plugin(data), mbInsideFunctionDecl(false) {}
+
+    virtual void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); }
+
+    // When warning about function params of primitive type that could be passed
+    // by value instead of by reference, make sure not to warn if the parameter
+    // is ever bound to a reference; on the one hand, this needs scaffolding in
+    // all Traverse*Decl functions (indirectly) derived from FunctionDecl; and
+    // on the other hand, use a hack of ignoring just the DeclRefExprs nested in
+    // LValueToRValue ImplicitCastExprs when determining whether a param is
+    // bound to a reference:
+    bool TraverseFunctionDecl(FunctionDecl *);
+    bool TraverseImplicitCastExpr(ImplicitCastExpr *);
+
+    bool VisitBinAssign(BinaryOperator const *);
+    bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *);
+
+private:
+    bool isFat(QualType type);
+
+    bool mbInsideFunctionDecl;
+    std::unordered_set<ParmVarDecl const *> mParamExclusions;
+};
+
+bool PassParamsByRef::TraverseFunctionDecl(FunctionDecl* functionDecl)
+{
+    if (ignoreLocation(functionDecl))
+        return true;
+    if (functionDecl->isDeleted()
+        || functionDecl->isFunctionTemplateSpecialization())
+    {
+        return true;
+    }
+    // only consider base declarations, not overridden ones, or we warn on methods that
+    // are overriding stuff from external libraries
+    const CXXMethodDecl * methodDecl = dyn_cast<CXXMethodDecl>(functionDecl);
+    if (methodDecl && methodDecl->size_overridden_methods() > 0)
+        return true;
+
+    // Only warn on the definition of the function:
+    if (!functionDecl->doesThisDeclarationHaveABody())
+        return true;
+
+    mbInsideFunctionDecl = true;
+    mParamExclusions.clear();
+    bool ret = RecursiveASTVisitor::TraverseFunctionDecl(functionDecl);
+    mbInsideFunctionDecl = false;
+
+    auto cxxConstructorDecl = dyn_cast<CXXConstructorDecl>(functionDecl);
+    unsigned n = functionDecl->getNumParams();
+    for (unsigned i = 0; i != n; ++i) {
+        const ParmVarDecl * pvDecl = functionDecl->getParamDecl(i);
+        auto const t = pvDecl->getType();
+        if (!isFat(t))
+            continue;
+        if (mParamExclusions.find(pvDecl) != mParamExclusions.end())
+            continue;
+        // Ignore cases where the parameter is std::move'd.
+        // This is a fairly simple check, might need some more complexity if the parameter is std::move'd
+        // somewhere else in the constructor.
+        bool bFoundMove = false;
+        if (cxxConstructorDecl) {
+            for (CXXCtorInitializer const * cxxCtorInitializer : cxxConstructorDecl->inits()) {
+                if (cxxCtorInitializer->isMemberInitializer())
+                {
+                    auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(cxxCtorInitializer->getInit()->IgnoreParenImpCasts());
+                    if (cxxConstructExpr && cxxConstructExpr->getNumArgs() == 1)
+                    {
+                        if (auto callExpr = dyn_cast<CallExpr>(cxxConstructExpr->getArg(0)->IgnoreParenImpCasts())) {
+                            if (loplugin::DeclCheck(callExpr->getCalleeDecl()).Function("move").StdNamespace()) {
+                                bFoundMove = true;
+                                break;
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        if (bFoundMove)
+            continue;
+        report(
+            DiagnosticsEngine::Warning,
+            ("passing %0 by value, rather pass by const lvalue reference"),
+            pvDecl->getLocation())
+            << t << pvDecl->getSourceRange();
+        auto can = functionDecl->getCanonicalDecl();
+        if (can->getLocation() != functionDecl->getLocation()) {
+            report(
+                DiagnosticsEngine::Note, "function is declared here:",
+                can->getLocation())
+                << can->getSourceRange();
+        }
+    }
+    return ret;
+}
+
+bool PassParamsByRef::TraverseImplicitCastExpr(ImplicitCastExpr * expr) {
+    if (ignoreLocation(expr))
+        return true;
+    return
+        (expr->getCastKind() == CK_LValueToRValue
+         && (dyn_cast<DeclRefExpr>(expr->getSubExpr()->IgnoreParenImpCasts())
+             != nullptr))
+        || RecursiveASTVisitor::TraverseImplicitCastExpr(expr);
+}
+
+bool PassParamsByRef::VisitBinAssign(const BinaryOperator * binaryOperator)
+{
+    if (!mbInsideFunctionDecl)
+        return true;
+    // if we are assigning to a parameter, it can be inconvenient to make the param pass-by-ref
+    if (auto declRefExpr = dyn_cast<DeclRefExpr>(binaryOperator->getLHS()))
+    {
+        if (auto parmVarDecl = dyn_cast<ParmVarDecl>(declRefExpr->getDecl()))
+            mParamExclusions.emplace(parmVarDecl);
+    }
+    return true;
+}
+
+bool PassParamsByRef::VisitCXXOperatorCallExpr(const CXXOperatorCallExpr * cxxOperatorCallExpr )
+{
+    if (!mbInsideFunctionDecl)
+        return true;
+    // if we are assigning to a parameter, it can be inconvenient to make the param pass-by-ref
+    if (!cxxOperatorCallExpr->isAssignmentOp())
+        return true;
+    auto declRefExpr = dyn_cast<DeclRefExpr>(cxxOperatorCallExpr->getArg(0));
+    if (!declRefExpr)
+        return true;
+    if (auto parmVarDecl = dyn_cast<ParmVarDecl>(declRefExpr->getDecl()))
+        mParamExclusions.emplace(parmVarDecl);
+    return true;
+}
+
+bool PassParamsByRef::isFat(QualType type) {
+    if (!type->isRecordType()) {
+        return false;
+    }
+    loplugin::TypeCheck tc(type);
+    if ((tc.Class("Reference").Namespace("uno").Namespace("star")
+            .Namespace("sun").Namespace("com").GlobalNamespace())
+        || (tc.Class("Sequence").Namespace("uno").Namespace("star")
+            .Namespace("sun").Namespace("com").GlobalNamespace())
+        || tc.Class("OString").Namespace("rtl").GlobalNamespace()
+        || tc.Class("OUString").Namespace("rtl").GlobalNamespace()
+        || tc.Class("Reference").Namespace("rtl").GlobalNamespace())
+    {
+        return true;
+    }
+    if (type->isIncompleteType()) {
+        return false;
+    }
+    Type const * t2 = type.getTypePtrOrNull();
+    return t2 != nullptr
+        && compiler.getASTContext().getTypeSizeInChars(t2).getQuantity() > 64;
+}
+
+loplugin::Plugin::Registration< PassParamsByRef > X("passparamsbyref");
+
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */

compilerplugins/clang/passstuffbyref.cxx

@@ -31,7 +31,7 @@ class PassStuffByRef:
     public RecursiveASTVisitor<PassStuffByRef>, public loplugin::Plugin
 {
 public:
-    explicit PassStuffByRef(InstantiationData const & data): Plugin(data), mbInsideFunctionDecl(false), mbFoundDisqualifier(false) {}
+    explicit PassStuffByRef(InstantiationData const & data): Plugin(data), mbInsideFunctionDecl(false), mbFoundReturnValueDisqualifier(false) {}
 
     virtual void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); }
 
@@ -59,12 +59,11 @@ private:
         T * decl, bool (RecursiveASTVisitor::* fn)(T *));
     void checkParams(const FunctionDecl * functionDecl);
     void checkReturnValue(const FunctionDecl * functionDecl, const CXXMethodDecl * methodDecl);
-    bool isFat(QualType type);
     bool isPrimitiveConstRef(QualType type);
     bool isReturnExprDisqualified(const Expr* expr);
 
     bool mbInsideFunctionDecl;
-    bool mbFoundDisqualifier;
+    bool mbFoundReturnValueDisqualifier;
 
     struct FDecl {
         std::set<ParmVarDecl const *> parms;
@@ -185,51 +184,6 @@ void PassStuffByRef::checkParams(const FunctionDecl * functionDecl) {
     if (!functionDecl->doesThisDeclarationHaveABody()) {
         return;
     }
-    auto cxxConstructorDecl = dyn_cast<CXXConstructorDecl>(functionDecl);
-    unsigned n = functionDecl->getNumParams();
-    for (unsigned i = 0; i != n; ++i) {
-        const ParmVarDecl * pvDecl = functionDecl->getParamDecl(i);
-        auto const t = pvDecl->getType();
-        if (!isFat(t)) {
-            continue;
-        }
-        // Ignore cases where the parameter is std::move'd.
-        // This is a fairly simple check, might need some more complexity if the parameter is std::move'd
-        // somewhere else in the constructor.
-        bool bFoundMove = false;
-        if (cxxConstructorDecl) {
-            for (CXXCtorInitializer const * cxxCtorInitializer : cxxConstructorDecl->inits()) {
-                if (cxxCtorInitializer->isMemberInitializer())
-                {
-                    auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(cxxCtorInitializer->getInit()->IgnoreParenImpCasts());
-                    if (cxxConstructExpr && cxxConstructExpr->getNumArgs() == 1)
-                    {
-                        if (auto callExpr = dyn_cast<CallExpr>(cxxConstructExpr->getArg(0)->IgnoreParenImpCasts())) {
-                            if (loplugin::DeclCheck(callExpr->getCalleeDecl()).Function("move").StdNamespace()) {
-                                bFoundMove = true;
-                                break;
-                            }
-                        }
-                    }
-                }
-            }
-        }
-        if (bFoundMove) {
-            continue;
-        }
-        report(
-            DiagnosticsEngine::Warning,
-            ("passing %0 by value, rather pass by const lvalue reference"),
-            pvDecl->getLocation())
-            << t << pvDecl->getSourceRange();
-        auto can = functionDecl->getCanonicalDecl();
-        if (can->getLocation() != functionDecl->getLocation()) {
-            report(
-                DiagnosticsEngine::Note, "function is declared here:",
-                can->getLocation())
-                << can->getSourceRange();
-        }
-    }
     // ignore stuff that forms part of the stable URE interface
     if (isInUnoIncludeFile(functionDecl)) {
         return;
@@ -241,6 +195,7 @@ void PassStuffByRef::checkParams(const FunctionDecl * functionDecl) {
     {
         return;
     }
+    unsigned n = functionDecl->getNumParams();
     assert(!functionDecls_.empty());
     functionDecls_.back().check = true;
     for (unsigned i = 0; i != n; ++i) {
@@ -311,11 +266,11 @@ void PassStuffByRef::checkReturnValue(const FunctionDecl * functionDecl, const C
         return;
     }
     mbInsideFunctionDecl = true;
-    mbFoundDisqualifier = false;
+    mbFoundReturnValueDisqualifier = false;
     TraverseStmt(functionDecl->getBody());
     mbInsideFunctionDecl = false;
 
-    if (mbFoundDisqualifier)
+    if (mbFoundReturnValueDisqualifier)
         return;
 
     report(
@@ -342,7 +297,7 @@ bool PassStuffByRef::VisitReturnStmt(const ReturnStmt * returnStmt)
     const Expr* expr = dyn_cast<Expr>(*returnStmt->child_begin())->IgnoreParenCasts();
 
     if (isReturnExprDisqualified(expr)) {
-        mbFoundDisqualifier = true;
+        mbFoundReturnValueDisqualifier = true;
         return true;
     }
     return true;
@@ -394,74 +349,20 @@ bool PassStuffByRef::VisitVarDecl(const VarDecl * varDecl)
     // things guarded by locking are probably best left alone
     loplugin::TypeCheck dc(varDecl->getType());
     if (dc.Class("Guard").Namespace("osl").GlobalNamespace())
-        mbFoundDisqualifier = true;
+        mbFoundReturnValueDisqualifier = true;
     if (dc.Class("ClearableGuard").Namespace("osl").GlobalNamespace())
-        mbFoundDisqualifier = true;
+        mbFoundReturnValueDisqualifier = true;
     if (dc.Class("ResettableGuard").Namespace("osl").GlobalNamespace())
-        mbFoundDisqualifier = true;
+        mbFoundReturnValueDisqualifier = true;
     else if (dc.Class("SolarMutexGuard").GlobalNamespace())
-        mbFoundDisqualifier = true;
+        mbFoundReturnValueDisqualifier = true;
     else if (dc.Class("SfxModelGuard").GlobalNamespace())
-        mbFoundDisqualifier = true;
+        mbFoundReturnValueDisqualifier = true;
     else if (dc.Class("ReadWriteGuard").Namespace("utl").GlobalNamespace())
-        mbFoundDisqualifier = true;
+        mbFoundReturnValueDisqualifier = true;
     return true;
 }
 
-// Would produce a wrong recommendation for
-//
-//   PresenterFrameworkObserver::RunOnUpdateEnd(
-//       xCC,
-//       [pSelf](bool){ return pSelf->ShutdownPresenterScreen(); });
-//
-// in PresenterScreen::RequestShutdownPresenterScreen
-// (sdext/source/presenter/PresenterScreen.cxx), with no obvious way to work
-// around it:
-//
-// bool PassStuffByRef::VisitLambdaExpr(const LambdaExpr * expr) {
-//     if (ignoreLocation(expr)) {
-//         return true;
-//     }
-//     for (auto i(expr->capture_begin()); i != expr->capture_end(); ++i) {
-//         if (i->getCaptureKind() == LambdaCaptureKind::LCK_ByCopy) {
-//             auto const t = i->getCapturedVar()->getType();
-//             if (isFat(t)) {
-//                 report(
-//                     DiagnosticsEngine::Warning,
-//                     ("%0 capture of %1 variable by copy, rather use capture"
-//                      " by reference---UNLESS THE LAMBDA OUTLIVES THE VARIABLE"),
-//                     i->getLocation())
-//                     << (i->isImplicit() ? "implicit" : "explicit") << t
-//                     << expr->getSourceRange();
-//             }
-//         }
-//     }
-//     return true;
-// }
-
-bool PassStuffByRef::isFat(QualType type) {
-    if (!type->isRecordType()) {
-        return false;
-    }
-    loplugin::TypeCheck tc(type);
-    if ((tc.Class("Reference").Namespace("uno").Namespace("star")
-            .Namespace("sun").Namespace("com").GlobalNamespace())
-        || (tc.Class("Sequence").Namespace("uno").Namespace("star")
-            .Namespace("sun").Namespace("com").GlobalNamespace())
-        || tc.Class("OString").Namespace("rtl").GlobalNamespace()
-        || tc.Class("OUString").Namespace("rtl").GlobalNamespace()
-        || tc.Class("Reference").Namespace("rtl").GlobalNamespace())
-    {
-        return true;
-    }
-    if (type->isIncompleteType()) {
-        return false;
-    }
-    Type const * t2 = type.getTypePtrOrNull();
-    return t2 != nullptr
-        && compiler.getASTContext().getTypeSizeInChars(t2).getQuantity() > 64;
-}
-
 bool PassStuffByRef::isPrimitiveConstRef(QualType type) {
     if (type->isIncompleteType()) {
         return false;

compilerplugins/clang/test/passparamsbyref.cxx

@@ -0,0 +1,39 @@
+/* -*- 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 <rtl/ustring.hxx>
+
+struct S {
+    OUString mv1;
+    OUString mv2;
+
+    // make sure we ignore cases where the passed in parameter is std::move'd
+    S(OUString v1, OUString v2)
+      : mv1(std::move(v1)), mv2((std::move(v2))) {}
+};
+
+
+void f()
+{
+    S* s;
+    OUString v1, v2;
+    s = new S(v1, v2);
+}
+
+
+// check that we don't warn when the param is modified
+OUString trim_string(OUString aString)
+{
+    aString += "xxx";
+    return aString;
+}
+
+// expected-no-diagnostics
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */

solenv/CompilerTest_compilerplugins_clang.mk

@@ -23,6 +23,7 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
     compilerplugins/clang/test/oslendian-1 \
     compilerplugins/clang/test/oslendian-2 \
     compilerplugins/clang/test/oslendian-3 \
+    compilerplugins/clang/test/passparamsbyref \
     compilerplugins/clang/test/passstuffbyref \
     compilerplugins/clang/test/redundantcast \
     compilerplugins/clang/test/redundantcopy \