loplugin:passstuffbyref improved return in canvas and svtools

and for now, ignore methods with params so we don't fall into the trap
of thinking that calls to methods like:
   Bar& foo(Bar &p) { return p; }
can be converted from
   Bar f() { return foo(Bar()); }
to
Bar const & f() { return foo(Bar()); }

Change-Id: Ia3795eb2baf353cb6bec4ebf40451f2789d66ad7
Reviewed-on: https://gerrit.libreoffice.org/47034
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 9 insertions, 0 deletions

diff --git a/compilerplugins/clang/passstuffbyref.cxx b/compilerplugins/clang/passstuffbyref.cxx
index 0630beb80b59..e152acec8673 100644
--- a/compilerplugins/clang/passstuffbyref.cxx
+++ b/compilerplugins/clang/passstuffbyref.cxx
@@ -402,6 +402,12 @@ bool PassStuffByRef::isReturnExprDisqualified(const Expr* expr)
             FunctionDecl const * calleeFunctionDecl = callExpr->getDirectCallee();
             if (!calleeFunctionDecl)
                 return true;
+            // TODO anything takes a param is suspect because it might return the param by ref.
+            // we could tighten this to only reject functions that have a param of the same type
+            // as the return type. Or we could check for such functions and disallow them.
+            // Or we could force such functions to be annotated somehow.
+            if (calleeFunctionDecl->getNumParams() > 0)
+                return true;
             auto tc = loplugin::TypeCheck(calleeFunctionDecl->getReturnType());
             if (!tc.LvalueReference() && !tc.Pointer())
                 return true;
diff --git a/compilerplugins/clang/test/passstuffbyref.cxx b/compilerplugins/clang/test/passstuffbyref.cxx
index 99e4a5a189b6..ed86d4d309d9 100644
--- a/compilerplugins/clang/test/passstuffbyref.cxx
+++ b/compilerplugins/clang/test/passstuffbyref.cxx
@@ -15,6 +15,7 @@
 struct S1 {
     OUString mv1;
     OUString const & get() const { return mv1; }
+    OUString const & get2(bool) const { return mv1; }
 };
 struct S2 {
     OUString mv1;
@@ -40,6 +41,8 @@ struct S2 {
     // TODO
     OUString get10() { return OUString(*&get6()); } // todoexpected-error {{rather return class rtl::OUString by const& than by value, to avoid unnecessary copying [loplugin:passstuffbyref]}}
     OUString get11() const { return mxCow->get(); } // expected-error {{rather return class rtl::OUString by const& than by value, to avoid unnecessary copying [loplugin:passstuffbyref]}}
+    // TODO anything takes a param is suspect because it might return the param by ref
+    OUString get12() { return child.get2(false); } // todoexpected-error {{rather return class rtl::OUString by const& than by value, to avoid unnecessary copying [loplugin:passstuffbyref]}}
 
     // no warning expected
     OUString set1() { return OUString("xxx"); }