diff options
author | Noel Grandin <noel.grandin@collabora.co.uk> | 2020-04-21 07:51:25 +0200 |
---|---|---|
committer | Noel Grandin <noel.grandin@collabora.co.uk> | 2020-05-01 08:26:24 +0200 |
commit | ed8152b1ed9baf859966fd21d6641dfba9c4467c (patch) | |
tree | b4f7b372433c5da3b8df41d026ff95fecece9ce6 /compilerplugins | |
parent | 6cb9b06432434fb3257118743780828b3b57326a (diff) |
improve loplugin:makeshared
to find places where we are converting stuff to unique_ptr
instead of using std::make_shared.
As a bonus, this tends to find places where we are using shared_ptr
where we can instead be using unique_ptr avoiding the locking overhead.
Change-Id: I1b57bbc4a6c766b48bba8e25a55161800e149f62
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/93207
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Diffstat (limited to 'compilerplugins')
-rw-r--r-- | compilerplugins/clang/makeshared.cxx | 124 | ||||
-rw-r--r-- | compilerplugins/clang/test/makeshared.cxx | 30 |
2 files changed, 138 insertions, 16 deletions
diff --git a/compilerplugins/clang/makeshared.cxx b/compilerplugins/clang/makeshared.cxx index 398a3acc4654..9f12b6c3bd6b 100644 --- a/compilerplugins/clang/makeshared.cxx +++ b/compilerplugins/clang/makeshared.cxx @@ -47,6 +47,43 @@ public: return false; if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/filter/excel/xecontent.cxx")) return false; + // no idea what is going on here + if (loplugin::isSamePathname(fn, SRCDIR "/svx/source/sidebar/nbdtmg.cxx")) + return false; + + // legitimate use of moving std::unique_ptr to std::shared_ptr + if (loplugin::isSamePathname(fn, SRCDIR "/comphelper/source/container/enumerablemap.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/svl/source/items/style.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/vcl/source/app/weldutils.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sfx2/source/appl/appopen.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/svx/source/table/tablertfimporter.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/docshell/externalrefmgr.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/attr/swatrset.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/condformat/condformatdlg.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/layout/frmtool.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/filter/excel/xihelper.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/filter/excel/xeformula.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/filter/excel/xichart.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/filter/html/htmlpars.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/view/cellsh1.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sw/source/filter/html/htmltab.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sw/source/filter/ww8/docxattributeoutput.cxx")) + return false; return true; } @@ -60,6 +97,8 @@ public: bool VisitCXXConstructExpr(CXXConstructExpr const*); bool VisitCXXMemberCallExpr(CXXMemberCallExpr const*); + bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr const*); + bool VisitVarDecl(VarDecl const*); }; bool MakeShared::VisitCXXConstructExpr(CXXConstructExpr const* constructExpr) @@ -71,25 +110,39 @@ bool MakeShared::VisitCXXConstructExpr(CXXConstructExpr const* constructExpr) if (!(constructExpr->getNumArgs() == 1 || (constructExpr->getNumArgs() > 1 && isa<CXXDefaultArgExpr>(constructExpr->getArg(1))))) return true; - auto cxxNewExpr = dyn_cast<CXXNewExpr>(constructExpr->getArg(0)->IgnoreParenImpCasts()); - if (!cxxNewExpr) - return true; - auto construct2 = cxxNewExpr->getConstructExpr(); - if (construct2) + auto arg0 = constructExpr->getArg(0)->IgnoreParenImpCasts(); + auto cxxNewExpr = dyn_cast<CXXNewExpr>(arg0); + if (cxxNewExpr) { - if (construct2->getConstructor()->getAccess() != AS_public) - return true; - if (construct2->getNumArgs() == 1 && isa<CXXStdInitializerListExpr>(construct2->getArg(0))) - return true; + auto construct2 = cxxNewExpr->getConstructExpr(); + if (construct2) + { + if (construct2->getConstructor()->getAccess() != AS_public) + return true; + if (construct2->getNumArgs() == 1 + && isa<CXXStdInitializerListExpr>(construct2->getArg(0))) + return true; + } } + else if (loplugin::TypeCheck(arg0->getType()).ClassOrStruct("shared_ptr").StdNamespace()) + return true; + else if (loplugin::TypeCheck(arg0->getType()).ClassOrStruct("weak_ptr").StdNamespace()) + return true; + else if (arg0->getType()->isDependentType()) + return true; + else if (isa<CXXNullPtrLiteralExpr>(arg0)) + return true; StringRef fn = getFilenameOfLocation( compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(constructExpr))); if (loplugin::isSamePathname(fn, SRCDIR "/include/o3tl/make_shared.hxx")) return true; + if (loplugin::isSamePathname(fn, SRCDIR "/svl/source/items/stylepool.cxx")) + return true; - report(DiagnosticsEngine::Warning, "rather use make_shared", compat::getBeginLoc(cxxNewExpr)) - << cxxNewExpr->getSourceRange(); + report(DiagnosticsEngine::Warning, "rather use make_shared than constructing from %0", + compat::getBeginLoc(constructExpr)) + << arg0->getType() << constructExpr->getSourceRange(); return true; } @@ -97,6 +150,7 @@ bool MakeShared::VisitCXXMemberCallExpr(CXXMemberCallExpr const* cxxMemberCallEx { if (ignoreLocation(cxxMemberCallExpr)) return true; + if (cxxMemberCallExpr->getNumArgs() != 1) return true; @@ -132,6 +186,54 @@ bool MakeShared::VisitCXXMemberCallExpr(CXXMemberCallExpr const* cxxMemberCallEx return true; } +bool MakeShared::VisitCXXOperatorCallExpr(CXXOperatorCallExpr const* operCallExpr) +{ + if (ignoreLocation(operCallExpr)) + return true; + if (!operCallExpr->isAssignmentOp()) + return true; + + if (!loplugin::TypeCheck(operCallExpr->getType()).ClassOrStruct("shared_ptr").StdNamespace()) + return true; + + if (loplugin::TypeCheck(operCallExpr->getArg(1)->getType()) + .ClassOrStruct("shared_ptr") + .StdNamespace()) + return true; + + report(DiagnosticsEngine::Warning, "rather use make_shared than constructing from %0", + compat::getBeginLoc(operCallExpr)) + << operCallExpr->getArg(1)->getType() << operCallExpr->getSourceRange(); + + return true; +} + +bool MakeShared::VisitVarDecl(VarDecl const* varDecl) +{ + if (ignoreLocation(varDecl)) + return true; + if (!varDecl->hasInit()) + return true; + + if (!loplugin::TypeCheck(varDecl->getType()).ClassOrStruct("shared_ptr").StdNamespace()) + return true; + + if (varDecl->getInit()->getType().isNull()) + return true; + if (varDecl->getInit()->getType()->isDependentType()) + return true; + if (loplugin::TypeCheck(varDecl->getInit()->IgnoreParenImpCasts()->getType()) + .ClassOrStruct("shared_ptr") + .StdNamespace()) + return true; + + report(DiagnosticsEngine::Warning, "rather use make_shared than constructing from %0", + compat::getBeginLoc(varDecl)) + << varDecl->getInit()->getType() << varDecl->getSourceRange(); + + return true; +} + loplugin::Plugin::Registration<MakeShared> makeshared("makeshared"); } // namespace diff --git a/compilerplugins/clang/test/makeshared.cxx b/compilerplugins/clang/test/makeshared.cxx index 3bb4702a05d8..8833928e8c18 100644 --- a/compilerplugins/clang/test/makeshared.cxx +++ b/compilerplugins/clang/test/makeshared.cxx @@ -23,11 +23,12 @@ private: void test1() { - std::shared_ptr<int> x( - new int); // expected-error {{rather use make_shared [loplugin:makeshared]}} - x.reset(new int); // expected-error {{rather use make_shared [loplugin:makeshared]}} - x = std::shared_ptr<int>( - new int); // expected-error {{rather use make_shared [loplugin:makeshared]}} + // expected-error@+1 {{rather use make_shared than constructing from 'int *' [loplugin:makeshared]}} + std::shared_ptr<int> x(new int); + // expected-error@+1 {{rather use make_shared [loplugin:makeshared]}} + x.reset(new int); + // expected-error@+1 {{rather use make_shared than constructing from 'int *' [loplugin:makeshared]}} + x = std::shared_ptr<int>(new int); // no warning expected std::shared_ptr<int> y(new int, o3tl::default_delete<int>()); @@ -40,4 +41,23 @@ void test1() auto a = std::shared_ptr<o3tl::sorted_vector<int>>(new o3tl::sorted_vector<int>({ 1, 2 })); }; +void test2() +{ + // expected-error-re@+1 {{rather use make_shared than constructing from {{.+}} (aka 'unique_ptr<int>') [loplugin:makeshared]}} + std::shared_ptr<int> x = std::make_unique<int>(1); + // expected-error-re@+1 {{rather use make_shared than constructing from {{.+}} (aka 'unique_ptr<int>') [loplugin:makeshared]}} + x = std::make_unique<int>(1); + (void)x; + + // expected-error-re@+1 {{rather use make_shared than constructing from {{.+}} (aka 'unique_ptr<int>') [loplugin:makeshared]}} + std::shared_ptr<int> y(std::make_unique<int>(1)); + (void)y; + + std::unique_ptr<int> u1; + // expected-error-re@+1 {{rather use make_shared than constructing from {{.+}} (aka 'std::unique_ptr<int, std::default_delete<int> >') [loplugin:makeshared]}} + std::shared_ptr<int> z = std::move(u1); + // expected-error-re@+1 {{rather use make_shared than constructing from {{.+}} (aka 'std::unique_ptr<int, std::default_delete<int> >') [loplugin:makeshared]}} + z = std::move(u1); +} + /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ |