compilerplugins/clang/passstuffbyref.cxx | 79 ++++++++---
compilerplugins/clang/test/passstuffbyref.cxx | 2
configmgr/source/node.hxx | 2
extensions/source/abpilot/datasourcehandling.cxx | 4
extensions/source/abpilot/datasourcehandling.hxx | 2
include/drawinglayer/primitive2d/structuretagprimitive2d.hxx | 2
include/test/a11y/accessibletestbase.hxx | 2
sot/source/sdstor/ucbstorage.cxx | 12 -
toolkit/source/controls/table/tablecontrol_impl.cxx | 2
toolkit/source/controls/table/tablecontrol_impl.hxx | 2
10 files changed, 77 insertions(+), 32 deletions(-)
New commits:
commit 5fdfc074c93447f9eb1c9a351ee4690126ba782c
Author: Noel Grandin <noel.gran...@collabora.co.uk>
AuthorDate: Tue Nov 5 11:19:51 2024 +0200
Commit: Noel Grandin <noel.gran...@collabora.co.uk>
CommitDate: Thu Nov 7 17:20:09 2024 +0100
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.gran...@collabora.co.uk>
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();
