From 7573abfdef5b02f37d3bb7040a282f13e791101c Mon Sep 17 00:00:00 2001 From: Noel Grandin Date: Wed, 13 Apr 2016 08:27:17 +0200 Subject: update loplugin passstuffbyref to check return types of methods like Foo getFoo() const { return m_foo; } where we can rather do const Foo& getFoo() const { return m_foo; } and let the client code decide if it wants copy Foo. Inspired by a performance problem where we were unwittingly copy constructing a large struct repeatedly just so client code could interrogate the members of the struct. When all of the changes this plugin finds are applied, I find that 'perf stat make check' shows on average a 1.7% reduction in CPU cycles. Change-Id: Ic27b4f817aa98f2a2a009f2d4e4a962cbe9c613e --- compilerplugins/clang/passstuffbyref.cxx | 110 ++++++++++++++++++++++++------- 1 file changed, 88 insertions(+), 22 deletions(-) (limited to 'compilerplugins') diff --git a/compilerplugins/clang/passstuffbyref.cxx b/compilerplugins/clang/passstuffbyref.cxx index 9da28b3c5938..8c4006b43f8a 100644 --- a/compilerplugins/clang/passstuffbyref.cxx +++ b/compilerplugins/clang/passstuffbyref.cxx @@ -11,6 +11,7 @@ #include #include "plugin.hxx" +#include "compat.hxx" #include "typecheck.hxx" // Find places where various things are passed by value. @@ -35,44 +36,109 @@ public: virtual void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); } bool VisitFunctionDecl(const FunctionDecl * decl); - + bool VisitCallExpr(const CallExpr * ) { if (mbInsideFunctionDecl) mbFoundDisqualifier = true; return true; } + bool VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr * ) { if (mbInsideFunctionDecl) mbFoundDisqualifier = true; return true; } + bool VisitDeclStmt(const DeclStmt * ) { if (mbInsideFunctionDecl) mbFoundDisqualifier = true; return true; } bool VisitLambdaExpr(const LambdaExpr * expr); private: bool isFat(QualType type); + bool mbInsideFunctionDecl; + bool mbFoundDisqualifier; }; bool PassStuffByRef::VisitFunctionDecl(const FunctionDecl * functionDecl) { if (ignoreLocation(functionDecl)) { return true; } - // only warn on the definition/prototype of the function, - // not on the function implementation - if ((functionDecl->isThisDeclarationADefinition() - && functionDecl->getPreviousDecl() != nullptr) - || functionDecl->isDeleted()) - { + if (functionDecl->isDeleted()) return true; - } // only consider base declarations, not overriden ones, or we warn on methods that // are overriding stuff from external libraries - if (isa(functionDecl)) { - CXXMethodDecl const * m = dyn_cast(functionDecl); - if (m->size_overridden_methods() > 0) + const CXXMethodDecl * methodDecl = dyn_cast(functionDecl); + if (methodDecl && methodDecl->size_overridden_methods() > 0) { return true; } - 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)) { - report( - DiagnosticsEngine::Warning, - ("passing %0 by value, rather pass by const lvalue reference"), - pvDecl->getLocation()) - << t << pvDecl->getSourceRange(); + // only warn on the definition/prototype of the function, + // not on the function implementation + if (!functionDecl->isThisDeclarationADefinition()) + { + 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)) { + report( + DiagnosticsEngine::Warning, + ("passing %0 by value, rather pass by const lvalue reference"), + pvDecl->getLocation()) + << t << pvDecl->getSourceRange(); + } } + return true; + } + + + if (methodDecl && methodDecl->isVirtual()) { + return true; + } + if( !functionDecl->hasBody()) { + return true; + } + + const QualType type = functionDecl->getReturnType().getDesugaredType(compiler.getASTContext()); + if (type->isReferenceType() || type->isIntegralOrEnumerationType() || type->isPointerType() + || type->isTemplateTypeParmType() || type->isDependentType() || type->isBuiltinType() + || type->isScalarType()) + { + return true; + } + + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc( + functionDecl->getCanonicalDecl()->getNameInfo().getLoc()))) { + return true; + } + std::string aFunctionName = functionDecl->getQualifiedNameAsString(); + // function is passed as parameter to another function + if (aFunctionName == "GDIMetaFile::ImplColMonoFnc" + || aFunctionName == "editeng::SvxBorderLine::darkColor" + || aFunctionName.compare(0, 8, "xforms::") == 0) + return true; + // not sure how to exclude this yet, returns copy of one of it's params + if (aFunctionName == "sameDistColor" || aFunctionName == "sameColor" + || aFunctionName == "pcr::(anonymous namespace)::StringIdentity::operator()" + || aFunctionName == "matop::COp::operator()" + || aFunctionName == "slideshow::internal::accumulate" + || aFunctionName == "slideshow::internal::lerp") + return true; + // depends on a define + if (aFunctionName == "SfxObjectShell::GetSharedFileURL") + return true; + mbInsideFunctionDecl = true; + mbFoundDisqualifier = false; + TraverseStmt(functionDecl->getBody()); + mbInsideFunctionDecl = false; + + if (mbFoundDisqualifier) + return true; + + report( + DiagnosticsEngine::Warning, + "rather return %0 from function %1 %2 by const& than by value, to avoid unnecessary copying", + functionDecl->getSourceRange().getBegin()) + << type.getAsString() << aFunctionName << type->getTypeClassName() << functionDecl->getSourceRange(); + + // display the location of the class member declaration so I don't have to search for it by hand + if (functionDecl->getSourceRange().getBegin() != functionDecl->getCanonicalDecl()->getSourceRange().getBegin()) + { + report( + DiagnosticsEngine::Note, + "rather return by const& than by value", + functionDecl->getCanonicalDecl()->getSourceRange().getBegin()) + << functionDecl->getCanonicalDecl()->getSourceRange(); } + //functionDecl->dump(); return true; } @@ -119,7 +185,7 @@ bool PassStuffByRef::isFat(QualType type) { && compiler.getASTContext().getTypeSizeInChars(t2).getQuantity() > 64; } -loplugin::Plugin::Registration< PassStuffByRef > X("passstuffbyref"); +loplugin::Plugin::Registration< PassStuffByRef > X("passstuffbyref", false); } -- cgit