loplugin:passstuffbyref make some small improvements

Change-Id: Ib14a2e6b41165887fcf99c3d155850faa8564822
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/176218
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

10 files changed, 77 insertions, 32 deletions

diff --git a/compilerplugins/clang/passstuffbyref.cxx b/compilerplugins/clang/passstuffbyref.cxx
index 549987e43b53..265b250e13f2 100644
--- a/compilerplugins/clang/passstuffbyref.cxx
+++ b/compilerplugins/clang/passstuffbyref.cxx
@@ -189,12 +189,20 @@ void PassStuffByRef::checkParams(const FunctionDecl * functionDecl) {
         return;
     }
     // these functions are passed as parameters to another function
-    if (loplugin::DeclCheck(functionDecl).MemberFunction()
+    auto dc = loplugin::DeclCheck(functionDecl);
+    if (dc.MemberFunction()
         .Class("ShapeAttributeLayer").Namespace("internal")
         .Namespace("slideshow").GlobalNamespace())
     {
         return;
     }
+    // not sure why, but changing these causes problems
+    if (dc.Function("lcl_createColorMapFromShapeProps")
+        || dc.Function("getAPIAnglesFrom3DProperties").Class("Scene3DHelper")
+        || dc.Function("FillSeriesSimple").Class("ScTable")
+        || dc.Function("FillAutoSimple").Class("ScTable"))
+        return;
+
     unsigned n = functionDecl->getNumParams();
     assert(!functionDecls_.empty());
     functionDecls_.back().check = true;
@@ -282,28 +290,51 @@ void PassStuffByRef::checkReturnValue(const FunctionDecl * functionDecl, const C
         || dc.Function("parseSingleValue").AnonymousNamespace().Namespace("configmgr").GlobalNamespace()
         || dc.Function("Create").Class("HandlerComponentBase").Namespace("pcr").GlobalNamespace()
         || dc.Function("toAny").Struct("Convert").Namespace("detail").Namespace("comphelper").GlobalNamespace()
-        || dc.Function("makeAny").Namespace("utl").GlobalNamespace()) {
+        || dc.Function("makeAny").Namespace("utl").GlobalNamespace()
+        || dc.Operator(OO_Call).Struct("makeAny") // in chart2
+        || dc.Function("toString").Struct("assertion_traits"))
         return;
-    }
-    if (startswith(type.getAsString(), "struct o3tl::strong_int")) {
+    // these are sometimes dummy classes
+    if (dc.MemberFunction().Class("DdeService")
+        || dc.MemberFunction().Class("DdeConnection")
+        || dc.MemberFunction().Class("DdeTopic")
+        || dc.MemberFunction().Class("CrashReporter"))
         return;
-    }
+    // function has its address taken and is used as a function pointer value
+    if (dc.Function("xforms_bool"))
+        return;
+    if (startswith(type.getAsString(), "struct o3tl::strong_int"))
+        return;
+    // false positive
+    if (dc.Function("FindFieldSep").Namespace("mark").Namespace("sw"))
+        return;
+    // false positive
+    if (dc.Function("GetTime").Class("SwDateTimeField"))
+        return;
+    // false positive
+    if (dc.Function("Create").Class("StyleFamilyEntry"))
+        return;
+    if (dc.Function("getManualMax").Class("SparklineAttributes"))
+        return;
+    if (dc.Function("getManualMin").Class("SparklineAttributes"))
+        return;
+    if (dc.Function("ReadEmbeddedData").Class("XclImpHyperlink"))
+        return;
+
     auto tc = loplugin::TypeCheck(functionDecl->getReturnType());
+
     // these functions are passed by function-pointer
     if (functionDecl->getIdentifier() && functionDecl->getName() == "GetRanges"
         && tc.Struct("WhichRangesContainer").GlobalNamespace())
         return;
-    // extremely simple class, might as well pass by value
-    if (tc.Class("Color")) {
-        return;
-    }
-    // extremely simple class, might as well pass by value
-    if (tc.Struct("TranslateId")) {
-        return;
-    }
-    if (tc.Class("span").Namespace("o3tl")) {
+
+    // extremely simple classes, might as well pass by value
+    if (tc.Class("Color")
+        || tc.Struct("TranslateId")
+        || tc.Class("span").StdNamespace()
+        || tc.Class("strong_ordering").StdNamespace()
+        || tc.Struct("TokenId"))
         return;
-    }
 
     // functionDecl->dump();
 
@@ -318,7 +349,7 @@ void PassStuffByRef::checkReturnValue(const FunctionDecl * functionDecl, const C
     report( DiagnosticsEngine::Warning,
             "rather return %0 by const& than by value, to avoid unnecessary copying",
             functionDecl->getSourceRange().getBegin())
-        << type.getAsString() << functionDecl->getSourceRange();
+        << type.getAsString();
 
     // display the location of the class member declaration so I don't have to search for it by hand
     auto canonicalDecl = functionDecl->getCanonicalDecl();
@@ -337,7 +368,12 @@ bool PassStuffByRef::VisitReturnStmt(const ReturnStmt * returnStmt)
 {
     if (!mbInsideFunctionDecl)
         return true;
-    const Expr* expr = dyn_cast<Expr>(*returnStmt->child_begin())->IgnoreParenImpCasts();
+    if (returnStmt->child_begin() == returnStmt->child_end())
+        return true;
+    auto expr = dyn_cast<Expr>(*returnStmt->child_begin());
+    if (!expr)
+        return true;
+    expr = expr->IgnoreParenImpCasts();
 
     if (isReturnExprDisqualified(expr))
         mbFoundReturnValueDisqualifier = true;
@@ -379,7 +415,13 @@ bool PassStuffByRef::isReturnExprDisqualified(const Expr* expr)
         if (isa<MaterializeTemporaryExpr>(expr)) {
             return true;
         }
-        if (isa<CXXBindTemporaryExpr>(expr)) {
+        if (auto bindExpr = dyn_cast<CXXBindTemporaryExpr>(expr)) {
+            // treat "return OUString();" as fine, because that is easy to convert
+            // to use EMPTY_OUSTRING.
+            if (loplugin::TypeCheck(expr->getType()).Class("OUString"))
+                if (auto tempExpr = dyn_cast<CXXTemporaryObjectExpr>(bindExpr->getSubExpr()))
+                    if (tempExpr->getNumArgs() == 0)
+                        return false;
             return true;
         }
         if (isa<InitListExpr>(expr)) {
@@ -474,6 +516,7 @@ bool PassStuffByRef::VisitVarDecl(const VarDecl * varDecl)
         dc.Class("SolarMutexGuard").GlobalNamespace() ||
         dc.Class("SfxModelGuard").GlobalNamespace() ||
         dc.Class("ReadWriteGuard").Namespace("utl").GlobalNamespace() ||
+        dc.Class("LifeTimeGuard").Namespace("apphelper").GlobalNamespace() || // in chart2
         dc.Class("unique_lock").StdNamespace() ||
         dc.Class("lock_guard").StdNamespace() ||
         dc.Class("scoped_lock").StdNamespace())
diff --git a/compilerplugins/clang/test/passstuffbyref.cxx b/compilerplugins/clang/test/passstuffbyref.cxx
index d90d6f05ba9f..08d91aeb1521 100644
--- a/compilerplugins/clang/test/passstuffbyref.cxx
+++ b/compilerplugins/clang/test/passstuffbyref.cxx
@@ -45,6 +45,8 @@ struct S2 {
     OUString get11() const { return mxCow->get(); } // expected-error {{rather return class rtl::OUString by const& than by value, to avoid unnecessary copying [loplugin:passstuffbyref]}}
     OUString get12() { return child.get2(false); } // expected-error {{rather return class rtl::OUString by const& than by value, to avoid unnecessary copying [loplugin:passstuffbyref]}}
 
+    OUString get13() { return OUString(); } // expected-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"); }
     OUString set2() { OUString v1("xxx"); return v1; }
diff --git a/configmgr/source/node.hxx b/configmgr/source/node.hxx
index cce8e3d4abb3..df5a127cdbb5 100644
--- a/configmgr/source/node.hxx
+++ b/configmgr/source/node.hxx
@@ -54,7 +54,7 @@ public:
     int getFinalized() const { return finalized_;}
 
     void setDescription(OUString const& description) { description_ = description; };
-    OUString getDescription() { return description_; }
+    const OUString & getDescription() { return description_; }
 
     rtl::Reference< Node > getMember(OUString const & name);
 
diff --git a/extensions/source/abpilot/datasourcehandling.cxx b/extensions/source/abpilot/datasourcehandling.cxx
index fb65c997187a..82013fecea3d 100644
--- a/extensions/source/abpilot/datasourcehandling.cxx
+++ b/extensions/source/abpilot/datasourcehandling.cxx
@@ -441,10 +441,10 @@ namespace abp
     }
 
 
-    OUString ODataSource::getName() const
+    const OUString & ODataSource::getName() const
     {
         if ( !isValid() )
-            return OUString();
+            return EMPTY_OUSTRING;
         return m_pImpl->sName;
     }
 
diff --git a/extensions/source/abpilot/datasourcehandling.hxx b/extensions/source/abpilot/datasourcehandling.hxx
index c6058e45e30a..4a755096a8d3 100644
--- a/extensions/source/abpilot/datasourcehandling.hxx
+++ b/extensions/source/abpilot/datasourcehandling.hxx
@@ -123,7 +123,7 @@ namespace abp
             // TODO: put this into the context class
 
         /// returns the name of the data source
-        OUString
+        const OUString &
                     getName() const;
 
         /// renames the data source
diff --git a/include/drawinglayer/primitive2d/structuretagprimitive2d.hxx b/include/drawinglayer/primitive2d/structuretagprimitive2d.hxx
index 3cc489973c19..15890959b004 100644
--- a/include/drawinglayer/primitive2d/structuretagprimitive2d.hxx
+++ b/include/drawinglayer/primitive2d/structuretagprimitive2d.hxx
@@ -74,7 +74,7 @@ namespace drawinglayer::primitive2d
             bool isDecorative() const { return mbIsDecorative; }
             bool isTaggedSdrObject() const;
             void const* GetAnchorStructureElementKey() const { return m_pAnchorStructureElementKey; }
-            ::std::vector<sal_Int32> GetAnnotIds() const { return m_AnnotIds; }
+            const ::std::vector<sal_Int32> & GetAnnotIds() const { return m_AnnotIds; }
 
             /// compare operator
             virtual bool operator==(const BasePrimitive2D& rPrimitive) const override;
diff --git a/include/test/a11y/accessibletestbase.hxx b/include/test/a11y/accessibletestbase.hxx
index 792012e5e22b..3c87005c12fb 100644
--- a/include/test/a11y/accessibletestbase.hxx
+++ b/include/test/a11y/accessibletestbase.hxx
@@ -212,7 +212,7 @@ protected:
 
         void setAutoClose(bool bAutoClose) { mbAutoClose = bAutoClose; }
 
-        css::uno::Reference<css::accessibility::XAccessible> getAccessible() const
+        const css::uno::Reference<css::accessibility::XAccessible>& getAccessible() const
         {
             return mxAccessible;
         }
diff --git a/sot/source/sdstor/ucbstorage.cxx b/sot/source/sdstor/ucbstorage.cxx
index 51c43bb290e3..178194bba87d 100644
--- a/sot/source/sdstor/ucbstorage.cxx
+++ b/sot/source/sdstor/ucbstorage.cxx
@@ -569,9 +569,9 @@ struct UCBStorageElement_Impl
 
     ::ucbhelper::Content*       GetContent();
     bool                        IsModified() const;
-    OUString                    GetContentType() const;
+    const OUString &            GetContentType() const;
     void                        SetContentType( const OUString& );
-    OUString                    GetOriginalContentType() const;
+    const OUString &            GetOriginalContentType() const;
     bool                        IsLoaded() const
                                 { return m_xStream.is() || m_xStorage.is(); }
 };
@@ -586,7 +586,7 @@ struct UCBStorageElement_Impl
         return nullptr;
 }
 
-OUString UCBStorageElement_Impl::GetContentType() const
+const OUString & UCBStorageElement_Impl::GetContentType() const
 {
     if ( m_xStream.is() )
         return m_xStream->m_aContentType;
@@ -595,7 +595,7 @@ OUString UCBStorageElement_Impl::GetContentType() const
     else
     {
         OSL_FAIL("Element not loaded!");
-        return OUString();
+        return EMPTY_OUSTRING;
     }
 }
 
@@ -612,14 +612,14 @@ void UCBStorageElement_Impl::SetContentType( const OUString& rType )
     }
 }
 
-OUString UCBStorageElement_Impl::GetOriginalContentType() const
+const OUString & UCBStorageElement_Impl::GetOriginalContentType() const
 {
     if ( m_xStream.is() )
         return m_xStream->m_aOriginalContentType;
     else if ( m_xStorage.is() )
         return m_xStorage->m_aOriginalContentType;
     else
-        return OUString();
+        return EMPTY_OUSTRING;
 }
 
 bool UCBStorageElement_Impl::IsModified() const
diff --git a/toolkit/source/controls/table/tablecontrol_impl.cxx b/toolkit/source/controls/table/tablecontrol_impl.cxx
index 99c7b815ca97..40820fbe68e2 100644
--- a/toolkit/source/controls/table/tablecontrol_impl.cxx
+++ b/toolkit/source/controls/table/tablecontrol_impl.cxx
@@ -2368,7 +2368,7 @@ namespace svt::table
     }
 
 
-    rtl::Reference<vcl::table::IAccessibleTableControl> TableControl_Impl::getAccessible( vcl::Window& i_parentWindow )
+    const rtl::Reference<vcl::table::IAccessibleTableControl> & TableControl_Impl::getAccessible( vcl::Window& i_parentWindow )
     {
         if (m_pAccessibleTable)
             return m_pAccessibleTable;
diff --git a/toolkit/source/controls/table/tablecontrol_impl.hxx b/toolkit/source/controls/table/tablecontrol_impl.hxx
index a9498958e796..bbfc56d1092e 100644
--- a/toolkit/source/controls/table/tablecontrol_impl.hxx
+++ b/toolkit/source/controls/table/tablecontrol_impl.hxx
@@ -280,7 +280,7 @@ namespace svt::table
         tools::Rectangle calcCellRect( sal_Int32 nRow, sal_Int32 nCol ) const;
 
         // A11Y
-        rtl::Reference<vcl::table::IAccessibleTableControl>
+        const rtl::Reference<vcl::table::IAccessibleTableControl> &
                         getAccessible( vcl::Window& i_parentWindow );
         void            disposeAccessible();