compilerplugins/clang/moveparam.cxx | 128 ++++++++++++++++++------ compilerplugins/clang/test/moveparam.cxx | 21 +++ vbahelper/source/vbahelper/vbadocumentsbase.cxx | 4 3 files changed, 117 insertions(+), 36 deletions(-)
New commits: commit d968425f009598bca3d10964c64f093b8d785c86 Author: Noel Grandin <[email protected]> AuthorDate: Tue Oct 12 09:56:59 2021 +0200 Commit: Noel Grandin <[email protected]> CommitDate: Tue Oct 12 13:12:11 2021 +0200 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 <[email protected]> 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: */ diff --git a/vbahelper/source/vbahelper/vbadocumentsbase.cxx b/vbahelper/source/vbahelper/vbadocumentsbase.cxx index b7b8124c2f0a..65ef6b6019e4 100644 --- a/vbahelper/source/vbahelper/vbadocumentsbase.cxx +++ b/vbahelper/source/vbahelper/vbadocumentsbase.cxx @@ -60,7 +60,7 @@ class DocumentsEnumImpl : public ::cppu::WeakImplHelper< container::XEnumeration public: /// @throws uno::RuntimeException - DocumentsEnumImpl( const uno::Reference< uno::XComponentContext >& xContext, const Documents& docs ) : m_xContext( xContext ), m_documents( docs ) + DocumentsEnumImpl( const uno::Reference< uno::XComponentContext >& xContext, Documents&& docs ) : m_xContext( xContext ), m_documents( std::move(docs) ) { m_it = m_documents.begin(); } @@ -138,7 +138,7 @@ public: //XEnumerationAccess virtual uno::Reference< container::XEnumeration > SAL_CALL createEnumeration( ) override { - return new DocumentsEnumImpl( m_xContext, m_documents ); + return new DocumentsEnumImpl( m_xContext, std::vector(m_documents) ); } // XIndexAccess virtual ::sal_Int32 SAL_CALL getCount( ) override
