summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorNoel Grandin <noel@peralex.com>2016-04-13 08:27:17 +0200
committerNoel Grandin <noel@peralex.com>2016-04-13 13:27:50 +0200
commit7573abfdef5b02f37d3bb7040a282f13e791101c (patch)
treefb336f017ab508ec75ec084f0dcc4a956db95736
parent97abbec95665b43a9a09e10a0fb31854cdbd5c0d (diff)
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
-rw-r--r--compilerplugins/clang/passstuffbyref.cxx110
1 files changed, 88 insertions, 22 deletions
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 <set>
#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<CXXMethodDecl>(functionDecl)) {
- CXXMethodDecl const * m = dyn_cast<CXXMethodDecl>(functionDecl);
- if (m->size_overridden_methods() > 0)
+ const CXXMethodDecl * methodDecl = dyn_cast<CXXMethodDecl>(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<type-parameter-0-0, svl::SharedString>::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);
}