improve loplugin passparamsbyref

I think I managed to disable this when I converted it to
use the shared plugin infrastructure.

So fix that, and then make it much smarter to avoid various
false positives.

Change-Id: I0a4657cff3b40a00434924bf764d024dbfd7d5b3
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/176646
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

3 files changed, 127 insertions, 29 deletions

diff --git a/compilerplugins/clang/passparamsbyref.cxx b/compilerplugins/clang/passparamsbyref.cxx
index 9a3562752c2c..1cdf5272390b 100644
--- a/compilerplugins/clang/passparamsbyref.cxx
+++ b/compilerplugins/clang/passparamsbyref.cxx
@@ -46,11 +46,19 @@ public:
     bool PreTraverseFunctionDecl(FunctionDecl *);
     bool PostTraverseFunctionDecl(FunctionDecl *, bool);
     bool TraverseFunctionDecl(FunctionDecl *);
+    bool PreTraverseCXXMethodDecl(CXXMethodDecl *);
+    bool PostTraverseCXXMethodDecl(CXXMethodDecl *, bool);
+    bool TraverseCXXMethodDecl(CXXMethodDecl *);
+    bool PreTraverseCXXConstructorDecl(CXXConstructorDecl *);
+    bool PostTraverseCXXConstructorDecl(CXXConstructorDecl *, bool);
+    bool TraverseCXXConstructorDecl(CXXConstructorDecl *);
     bool PreTraverseImplicitCastExpr(ImplicitCastExpr *);
     bool TraverseImplicitCastExpr(ImplicitCastExpr *);
 
     bool VisitBinaryOperator(BinaryOperator const *);
     bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *);
+    bool VisitCallExpr(const CallExpr *);
+    bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *);
 
 private:
     bool isFat(QualType type);
@@ -68,10 +76,10 @@ bool PassParamsByRef::PreTraverseFunctionDecl(FunctionDecl* functionDecl)
     {
         return false;
     }
-    // only consider base declarations, not overridden ones, or we warn on methods that
-    // are overriding stuff from external libraries
+    // Ignore virtual methods, sometimes we want to pass by value, and we cannot tell from
+    // the limited info available at an individual site.
     const CXXMethodDecl * methodDecl = dyn_cast<CXXMethodDecl>(functionDecl);
-    if (methodDecl && methodDecl->size_overridden_methods() > 0)
+    if (methodDecl && methodDecl->isVirtual())
         return false;
 
     // Only warn on the definition of the function:
@@ -87,7 +95,6 @@ bool PassParamsByRef::PostTraverseFunctionDecl(FunctionDecl* functionDecl, bool)
 {
     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);
@@ -96,29 +103,6 @@ bool PassParamsByRef::PostTraverseFunctionDecl(FunctionDecl* functionDecl, bool)
             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"),
@@ -146,6 +130,48 @@ bool PassParamsByRef::TraverseFunctionDecl(FunctionDecl* functionDecl)
     return ret;
 }
 
+bool PassParamsByRef::PreTraverseCXXMethodDecl(CXXMethodDecl* functionDecl)
+{
+    return PreTraverseFunctionDecl(functionDecl);
+}
+
+bool PassParamsByRef::PostTraverseCXXMethodDecl(CXXMethodDecl* functionDecl, bool b)
+{
+    return PostTraverseFunctionDecl(functionDecl, b);
+}
+
+bool PassParamsByRef::TraverseCXXMethodDecl(CXXMethodDecl* functionDecl)
+{
+    bool ret = true;
+    if (PreTraverseCXXMethodDecl(functionDecl))
+    {
+        ret = RecursiveASTVisitor::TraverseCXXMethodDecl(functionDecl);
+        PostTraverseCXXMethodDecl(functionDecl, ret);
+    }
+    return ret;
+}
+
+bool PassParamsByRef::PreTraverseCXXConstructorDecl(CXXConstructorDecl* functionDecl)
+{
+    return PreTraverseFunctionDecl(functionDecl);
+}
+
+bool PassParamsByRef::PostTraverseCXXConstructorDecl(CXXConstructorDecl* functionDecl, bool b)
+{
+    return PostTraverseFunctionDecl(functionDecl, b);
+}
+
+bool PassParamsByRef::TraverseCXXConstructorDecl(CXXConstructorDecl* functionDecl)
+{
+    bool ret = true;
+    if (PreTraverseCXXConstructorDecl(functionDecl))
+    {
+        ret = RecursiveASTVisitor::TraverseCXXConstructorDecl(functionDecl);
+        PostTraverseCXXConstructorDecl(functionDecl, ret);
+    }
+    return ret;
+}
+
 bool PassParamsByRef::PreTraverseImplicitCastExpr(ImplicitCastExpr * expr)
 {
     if (ignoreLocation(expr))
@@ -202,6 +228,50 @@ bool PassParamsByRef::VisitCXXOperatorCallExpr(const CXXOperatorCallExpr * cxxOp
     return true;
 }
 
+bool PassParamsByRef::VisitCallExpr(const CallExpr * callExpr )
+{
+    if (!mbInsideFunctionDecl)
+        return true;
+    if (loplugin::DeclCheck(callExpr->getCalleeDecl()).Function("move").StdNamespace())
+        if (auto declRefExpr = dyn_cast<DeclRefExpr>(callExpr->getArg(0)))
+        {
+            if (auto parmVarDecl = dyn_cast<ParmVarDecl>(declRefExpr->getDecl()))
+                mParamExclusions.emplace(parmVarDecl);
+        }
+    if (auto const fun = callExpr->getDirectCallee())
+    {
+        unsigned const n = std::min(fun->getNumParams(), callExpr->getNumArgs());
+        for (unsigned i = 0; i != n; ++i)
+        {
+            if (!loplugin::TypeCheck(fun->getParamDecl(i)->getType())
+                     .LvalueReference()
+                     .NonConstVolatile())
+                continue;
+            auto a = callExpr->getArg(i)->IgnoreParenImpCasts();
+            if (auto declRefExpr = dyn_cast<DeclRefExpr>(a))
+                if (auto parmVarDecl = dyn_cast<ParmVarDecl>(declRefExpr->getDecl()))
+                    mParamExclusions.emplace(parmVarDecl);
+        }
+    }
+    return true;
+}
+
+bool PassParamsByRef::VisitCXXMemberCallExpr(const CXXMemberCallExpr * callExpr )
+{
+    if (!mbInsideFunctionDecl)
+        return true;
+    // exclude cases where we call a non-const method on the parameter i.e. potentially mutating it
+    if (auto const e1 = callExpr->getMethodDecl())
+        if (!e1->isConst())
+        {
+            auto a = callExpr->getImplicitObjectArgument()->IgnoreParenImpCasts();
+            if (auto declRefExpr = dyn_cast<DeclRefExpr>(a))
+                if (auto parmVarDecl = dyn_cast<ParmVarDecl>(declRefExpr->getDecl()))
+                    mParamExclusions.emplace(parmVarDecl);
+        }
+    return true;
+}
+
 bool PassParamsByRef::isFat(QualType type) {
     if (!type->isRecordType()) {
         return false;
diff --git a/compilerplugins/clang/sharedvisitor/dummyplugin.hxx b/compilerplugins/clang/sharedvisitor/dummyplugin.hxx
index 1ef87416a907..8d48622e44ca 100644
--- a/compilerplugins/clang/sharedvisitor/dummyplugin.hxx
+++ b/compilerplugins/clang/sharedvisitor/dummyplugin.hxx
@@ -48,6 +48,8 @@ public:
     bool TraverseCXXCatchStmt( CXXCatchStmt* ) { return complain(); }
     bool TraverseCXXDestructorDecl( CXXDestructorDecl* ) { return complain(); }
     bool TraverseFunctionDecl( FunctionDecl* ) { return complain(); }
+    bool TraverseCXXMethodDecl( CXXMethodDecl* ) { return complain(); }
+    bool TraverseCXXConstructorDecl( CXXConstructorDecl* ) { return complain(); }
     bool TraverseSwitchStmt( SwitchStmt* ) { return complain(); }
     bool TraverseImplicitCastExpr( ImplicitCastExpr* ) { return complain(); }
     bool TraverseCStyleCastExpr( CStyleCastExpr* ) { return complain(); }
diff --git a/compilerplugins/clang/test/passparamsbyref.cxx b/compilerplugins/clang/test/passparamsbyref.cxx
index 010556a67b6e..707def635ca4 100644
--- a/compilerplugins/clang/test/passparamsbyref.cxx
+++ b/compilerplugins/clang/test/passparamsbyref.cxx
@@ -19,8 +19,36 @@ struct S {
     // 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))) {}
+
+    // expected-error-re@+1 {{passing '{{(rtl::)?}}OUString' by value, rather pass by const lvalue reference [loplugin:passparamsbyref]}}
+    S(OUString v1)
+        : mv1(v1) {}
+
+    // expected-error-re@+1 {{passing '{{(rtl::)?}}OUString' by value, rather pass by const lvalue reference [loplugin:passparamsbyref]}}
+    void foo(OUString v1) { (void) v1; }
+
+    // no warning expected
+    void foo2(OUString v1) { mv1 = std::move(v1); }
+
+    void takeByNonConstRef(OUString&);
+
+    // no warning expected
+    void foo3(OUString v)
+    {
+        takeByNonConstRef(v);
+    }
 };
 
+namespace test2
+{
+    struct TestObject { OUString s[64]; void nonConstMethod(); };
+
+    // no warning expected
+    void f1(TestObject to)
+    {
+        to.nonConstMethod();
+    }
+}
 
 void f()
 {
@@ -37,6 +65,4 @@ OUString trim_string(OUString aString)
     return aString;
 }
 
-// expected-no-diagnostics
-
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */