loplugin:moveparam check for collection params

Empirically, when we are passing a collection type to a constructor,
80% of the time, we are passing a local temporary that can be moved
instead of being copied.

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

2 files changed, 115 insertions, 34 deletions

diff --git a/compilerplugins/clang/moveparam.cxx b/compilerplugins/clang/moveparam.cxx
index 46816184071f..930e8a61a927 100644
--- a/compilerplugins/clang/moveparam.cxx
+++ b/compilerplugins/clang/moveparam.cxx
@@ -19,8 +19,13 @@
 #include "check.hxx"
 
 /*
-Look for places where we can pass by Primitive2DContainer param and so avoid
+Look for places where we can pass by move && param and so avoid
 unnecessary copies.
+Empirically, when we are passing a container type to a function, 80% of the time,
+we are passing a local temporary that can be moved instead of being copied.
+
+TODO this could be a lot smarter, with ignoring false+ e.g. when copying a param
+in a loop
 */
 
 namespace
@@ -33,7 +38,24 @@ public:
     {
     }
 
-    virtual bool preRun() override { return true; }
+    virtual bool preRun() override
+    {
+        std::string fn(handler.getMainFileName());
+        loplugin::normalizeDotDotInFilePath(fn);
+        if (loplugin::hasPathnamePrefix(fn, SRCDIR "/filter/source/msfilter/escherex.cxx"))
+            return false;
+        if (loplugin::hasPathnamePrefix(fn, SRCDIR "/sc/source/ui/docshell/docfunc.cxx"))
+            return false;
+        if (loplugin::hasPathnamePrefix(fn, SRCDIR "/sc/source/ui/view/viewfunc.cxx"))
+            return false;
+        if (loplugin::hasPathnamePrefix(fn, SRCDIR "/basegfx/source/polygon/b2dpolygontools.cxx"))
+            return false;
+        if (loplugin::hasPathnamePrefix(fn, SRCDIR "/basegfx/source/polygon/b3dpolygontools.cxx"))
+            return false;
+        if (loplugin::hasPathnamePrefix(fn, SRCDIR "/connectivity/source/commontools/dbtools.cxx"))
+            return false;
+        return true;
+    }
 
     virtual void run() override
     {
@@ -41,10 +63,10 @@ public:
             TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
     }
 
-    bool PreTraverseConstructorInitializer(CXXCtorInitializer*);
-    bool PostTraverseConstructorInitializer(CXXCtorInitializer*, bool);
-    bool TraverseConstructorInitializer(CXXCtorInitializer*);
     bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr*);
+    bool VisitCXXConstructExpr(const CXXConstructExpr*);
+
+    bool isContainerType(QualType qt);
 };
 
 bool MoveParam::VisitCXXOperatorCallExpr(const CXXOperatorCallExpr* callExpr)
@@ -53,9 +75,8 @@ bool MoveParam::VisitCXXOperatorCallExpr(const CXXOperatorCallExpr* callExpr)
         return true;
     if (!callExpr->isAssignmentOp())
         return true;
-    if (!loplugin::TypeCheck(callExpr->getType())
-             .Class("Primitive2DContainer")
-             .Namespace("primitive2d"))
+    auto qt = callExpr->getType();
+    if (!isContainerType(qt))
         return true;
     auto declRef = dyn_cast<DeclRefExpr>(callExpr->getArg(1)->IgnoreParenImpCasts());
     if (!declRef)
@@ -68,29 +89,29 @@ bool MoveParam::VisitCXXOperatorCallExpr(const CXXOperatorCallExpr* callExpr)
     if (!loplugin::TypeCheck(parmVarDecl->getType()).LvalueReference().Const())
         return true;
 
-    report(DiagnosticsEngine::Warning, "rather use move && param", compat::getBeginLoc(callExpr));
+    StringRef aFileName = getFilenameOfLocation(
+        compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(parmVarDecl)));
+    if (loplugin::hasPathnamePrefix(aFileName,
+                                    SRCDIR "/svx/source/sidebar/line/LineWidthValueSet.cxx"))
+        return true;
+
+    report(DiagnosticsEngine::Warning, "rather use move && param1", compat::getBeginLoc(callExpr));
 
     return true;
 }
 
-bool MoveParam::PreTraverseConstructorInitializer(CXXCtorInitializer* init)
+bool MoveParam::VisitCXXConstructExpr(const CXXConstructExpr* constructExpr)
 {
-    if (ignoreLocation(init->getSourceLocation()))
+    if (ignoreLocation(compat::getBeginLoc(constructExpr)))
         return true;
-    const FieldDecl* fieldDecl = init->getAnyMember();
-    if (!fieldDecl)
+    if (isInUnoIncludeFile(compat::getBeginLoc(constructExpr)))
         return true;
 
-    auto dc = loplugin::TypeCheck(fieldDecl->getType())
-                  .Class("Primitive2DContainer")
-                  .Namespace("primitive2d")
-                  .Namespace("drawinglayer")
-                  .GlobalNamespace();
-    if (!dc)
+    auto qt = constructExpr->getType();
+    if (!isContainerType(qt))
         return true;
 
-    auto constructExpr = dyn_cast_or_null<CXXConstructExpr>(init->getInit());
-    if (!constructExpr || constructExpr->getNumArgs() != 1)
+    if (constructExpr->getNumArgs() != 1)
         return true;
 
     auto declRef = dyn_cast<DeclRefExpr>(constructExpr->getArg(0)->IgnoreParenImpCasts());
@@ -104,23 +125,66 @@ bool MoveParam::PreTraverseConstructorInitializer(CXXCtorInitializer* init)
     if (!loplugin::TypeCheck(parmVarDecl->getType()).LvalueReference().Const())
         return true;
 
-    report(DiagnosticsEngine::Warning, "rather use move && param", init->getSourceLocation());
+    StringRef aFileName = getFilenameOfLocation(
+        compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(parmVarDecl)));
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR
+                                    "/include/drawinglayer/primitive2d/Primitive2DContainer.hxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName,
+                                    SRCDIR "/include/drawinglayer/primitive3d/baseprimitive3d.hxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/svx/source/svdraw/svdmrkv.cxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/editeng/swafopt.hxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(
+            aFileName, SRCDIR "/drawinglayer/source/primitive2d/textdecoratedprimitive2d.cxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName,
+                                    SRCDIR "/chart2/source/tools/InternalDataProvider.cxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sc/source/core/data/attrib.cxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sw/source/core/doc/docfmt.cxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/configmgr/source/modifications.cxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/svx/source/dialog/srchdlg.cxx"))
+        return true;
+    if (loplugin::hasPathnamePrefix(aFileName,
+                                    SRCDIR "/stoc/source/servicemanager/servicemanager.cxx"))
+        return true;
+
+    report(DiagnosticsEngine::Warning, "rather use move && param3",
+           compat::getBeginLoc(constructExpr));
 
     return true;
 }
-bool MoveParam::PostTraverseConstructorInitializer(CXXCtorInitializer*, bool) { return true; }
-bool MoveParam::TraverseConstructorInitializer(CXXCtorInitializer* init)
+
+bool MoveParam::isContainerType(QualType qt)
 {
-    bool ret = true;
-    if (PreTraverseConstructorInitializer(init))
-    {
-        ret = FilteringPlugin<MoveParam>::TraverseConstructorInitializer(init);
-        PostTraverseConstructorInitializer(init, ret);
-    }
-    return ret;
+    auto tc = loplugin::TypeCheck(qt);
+    return tc.Class("Primitive2DContainer")
+               .Namespace("primitive2d")
+               .Namespace("drawinglayer")
+               .GlobalNamespace()
+           || tc.ClassOrStruct("sorted_vector").Namespace("o3tl").GlobalNamespace()
+           || tc.ClassOrStruct("array").StdNamespace() || tc.ClassOrStruct("vector").StdNamespace()
+           || tc.ClassOrStruct("deque").StdNamespace()
+           || tc.ClassOrStruct("forward_list").StdNamespace()
+           || tc.ClassOrStruct("list").StdNamespace() || tc.ClassOrStruct("set").StdNamespace()
+           || tc.ClassOrStruct("map").StdNamespace() || tc.ClassOrStruct("multiset").StdNamespace()
+           || tc.ClassOrStruct("multimap").StdNamespace()
+           || tc.ClassOrStruct("unordered_set").StdNamespace()
+           || tc.ClassOrStruct("unordered_map").StdNamespace()
+           || tc.ClassOrStruct("unordered_multiset").StdNamespace()
+           || tc.ClassOrStruct("unordered_multimap").StdNamespace()
+           || tc.ClassOrStruct("stack").StdNamespace() || tc.ClassOrStruct("queue").StdNamespace()
+           || tc.ClassOrStruct("priority_queue").StdNamespace();
 }
 
-loplugin::Plugin::Registration<MoveParam> moveparam("moveparam", true);
+/** off by default because it needs some hand-holding */
+loplugin::Plugin::Registration<MoveParam> moveparam("moveparam", false);
 
 } // namespace
 
diff --git a/compilerplugins/clang/test/moveparam.cxx b/compilerplugins/clang/test/moveparam.cxx
index c90e8ae4bbd4..4e3df5b9c26a 100644
--- a/compilerplugins/clang/test/moveparam.cxx
+++ b/compilerplugins/clang/test/moveparam.cxx
@@ -9,6 +9,7 @@
 
 #include "config_clang.h"
 #include "o3tl/cow_wrapper.hxx"
+#include <map>
 
 namespace drawinglayer::primitive2d
 {
@@ -21,7 +22,7 @@ struct Foo
 {
     drawinglayer::primitive2d::Primitive2DContainer maMine;
 
-    // expected-error@+2 {{rather use move && param [loplugin:moveparam]}}
+    // expected-error@+2 {{rather use move && param3 [loplugin:moveparam]}}
     Foo(drawinglayer::primitive2d::Primitive2DContainer const& rContainer)
         : maMine(rContainer)
     {
@@ -35,9 +36,25 @@ struct Foo
 
     void foo1(const drawinglayer::primitive2d::Primitive2DContainer& rContainer)
     {
-        // expected-error@+1 {{rather use move && param [loplugin:moveparam]}}
+        // expected-error@+1 {{rather use move && param1 [loplugin:moveparam]}}
         maMine = rContainer;
     }
 };
 
+namespace test2
+{
+typedef std::map<int, int> Map2Map;
+
+struct Foo
+{
+    Map2Map maMine;
+
+    // expected-error@+2 {{rather use move && param3 [loplugin:moveparam]}}
+    Foo(Map2Map const& rContainer)
+        : maMine(rContainer)
+    {
+    }
+};
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */