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

@@ -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;
     }
+    if (functionDecl->isDeleted())
+        return true;
+    // only consider base declarations, not overriden ones, or we warn on methods that
+    // are overriding stuff from external libraries
+    const CXXMethodDecl * methodDecl = dyn_cast<CXXMethodDecl>(functionDecl);
+    if (methodDecl && methodDecl->size_overridden_methods() > 0) {
+            return true;
+    }
     // only warn on the definition/prototype of the function,
     // not on the function implementation
-    if ((functionDecl->isThisDeclarationADefinition()
+    if (!functionDecl->isThisDeclarationADefinition())
-         && functionDecl->getPreviousDecl() != nullptr)
+    {
-        || functionDecl->isDeleted())
+        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;
     }
-    // only consider base declarations, not overriden ones, or we warn on methods that
+
-    // are overriding stuff from external libraries
+    // ignore stuff that forms part of the stable URE interface
-    if (isa<CXXMethodDecl>(functionDecl)) {
+    if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc(
-        CXXMethodDecl const * m = dyn_cast<CXXMethodDecl>(functionDecl);
+                              functionDecl->getCanonicalDecl()->getNameInfo().getLoc()))) {
-        if (m->size_overridden_methods() > 0)
+        return true;
-            return true;
     }
-    unsigned n = functionDecl->getNumParams();
+    std::string aFunctionName = functionDecl->getQualifiedNameAsString();
-    for (unsigned i = 0; i != n; ++i) {
+    // function is passed as parameter to another function
-        const ParmVarDecl * pvDecl = functionDecl->getParamDecl(i);
+    if (aFunctionName == "GDIMetaFile::ImplColMonoFnc"
-        auto const t = pvDecl->getType();
+        || aFunctionName == "editeng::SvxBorderLine::darkColor"
-        if (isFat(t)) {
+        || aFunctionName.compare(0, 8, "xforms::") == 0)
-            report(
+        return true;
-                DiagnosticsEngine::Warning,
+    // not sure how to exclude this yet, returns copy of one of it's params
-                ("passing %0 by value, rather pass by const lvalue reference"),
+    if (aFunctionName == "sameDistColor" || aFunctionName == "sameColor"
-                pvDecl->getLocation())
+        || aFunctionName == "pcr::(anonymous namespace)::StringIdentity::operator()"
-                << t << pvDecl->getSourceRange();
+        || 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);
 
 }