compilerplugins/clang/refcounting.cxx | 68 +++++++++++++++++++++++++++++
compilerplugins/clang/test/refcounting.cxx | 23 ++++++++-
sd/source/ui/view/Outliner.cxx | 2
sw/source/uibase/uiview/uivwimp.cxx | 2
4 files changed, 89 insertions(+), 6 deletions(-)
New commits:
commit 32c8b03f6172acf3a19c5d1938b9935369f536f1
Author: Noel Grandin <noel.gran...@collabora.co.uk>
AuthorDate: Fri Jan 20 12:55:25 2023 +0200
Commit: Noel Grandin <noel.gran...@collabora.co.uk>
CommitDate: Fri Jan 20 12:58:40 2023 +0000
improve loplugin:refcounting
to catch places where we are converting a weak reference to a strong
reference, and then using a pointer to store the result
Change-Id: I69b132907b574e5c6974fadf18bd9658107d3a0d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/145877
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.gran...@collabora.co.uk>
diff --git a/compilerplugins/clang/refcounting.cxx
b/compilerplugins/clang/refcounting.cxx
index e65772f71e7d..ca6c0d97d9f0 100644
--- a/compilerplugins/clang/refcounting.cxx
+++ b/compilerplugins/clang/refcounting.cxx
@@ -75,6 +75,7 @@ private:
bool visitTemporaryObjectExpr(Expr const * expr);
bool isCastingReference(const Expr* expr);
+ bool isCallingGetOnWeakRef(const Expr* expr);
};
bool containsXInterfaceSubclass(const clang::Type* pType0);
@@ -714,6 +715,17 @@ bool RefCounting::VisitVarDecl(const VarDecl * varDecl) {
<< pointeeType
<< varDecl->getSourceRange();
}
+ if (isCallingGetOnWeakRef(varDecl->getInit()))
+ {
+ auto pointeeType = varDecl->getType()->getPointeeType();
+ if (containsOWeakObjectSubclass(pointeeType))
+ report(
+ DiagnosticsEngine::Warning,
+ "weak object being converted to strong, and then the
reference dropped, and managed via raw pointer, should be managed via
rtl::Reference",
+ varDecl->getLocation())
+ << pointeeType
+ << varDecl->getSourceRange();
+ }
}
return true;
}
@@ -762,6 +774,47 @@ bool RefCounting::isCastingReference(const Expr* expr)
return true;
}
+/**
+ Look for code like
+ makeFoo().get();
+ or
+ cast<T*>(makeFoo().get().get());
+ or
+ foo.get();
+ where makeFoo() returns a unotools::WeakReference<Foo>
+ and foo is a unotools::WeakReference<Foo> var.
+*/
+bool RefCounting::isCallingGetOnWeakRef(const Expr* expr)
+{
+ expr = expr->IgnoreImplicit();
+ // unwrap the cast (if any)
+ if (auto castExpr = dyn_cast<CastExpr>(expr))
+ expr = castExpr->getSubExpr()->IgnoreImplicit();
+ // unwrap outer get (if any)
+ if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(expr))
+ {
+ auto methodDecl = memberCallExpr->getMethodDecl();
+ if (methodDecl && methodDecl->getIdentifier() && methodDecl->getName()
== "get")
+ {
+ QualType objectType =
memberCallExpr->getImplicitObjectArgument()->getType();
+ if
(loplugin::TypeCheck(objectType).Class("Reference").Namespace("rtl"))
+ expr =
memberCallExpr->getImplicitObjectArgument()->IgnoreImplicit();
+ }
+ }
+ // check for converting a WeakReference to a strong reference via get()
+ if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(expr))
+ {
+ auto methodDecl = memberCallExpr->getMethodDecl();
+ if (methodDecl && methodDecl->getIdentifier() && methodDecl->getName()
== "get")
+ {
+ QualType objectType =
memberCallExpr->getImplicitObjectArgument()->getType();
+ if
(loplugin::TypeCheck(objectType).Class("WeakReference").Namespace("unotools"))
+ return true;
+ }
+ }
+ return false;
+}
+
bool RefCounting::VisitBinaryOperator(const BinaryOperator * binaryOperator)
{
if (ignoreLocation(binaryOperator))
@@ -801,6 +854,21 @@ bool RefCounting::VisitBinaryOperator(const BinaryOperator
* binaryOperator)
<< pointeeType
<< binaryOperator->getSourceRange();
}
+ if (isCallingGetOnWeakRef(binaryOperator->getRHS()))
+ {
+ // TODO Very dodgy code, but I see no simple way of fixing it
+ StringRef fileName =
getFilenameOfLocation(compiler.getSourceManager().getSpellingLoc(binaryOperator->getBeginLoc()));
+ if (loplugin::isSamePathname(fileName, SRCDIR
"/sd/source/ui/view/Outliner.cxx"))
+ return true;
+ auto pointeeType =
binaryOperator->getLHS()->getType()->getPointeeType();
+ if (containsOWeakObjectSubclass(pointeeType))
+ report(
+ DiagnosticsEngine::Warning,
+ "weak object being converted to strong, and then the reference
dropped, and managed via raw pointer, should be managed via rtl::Reference",
+ binaryOperator->getBeginLoc())
+ << pointeeType
+ << binaryOperator->getSourceRange();
+ }
return true;
}
diff --git a/compilerplugins/clang/test/refcounting.cxx
b/compilerplugins/clang/test/refcounting.cxx
index 2b8ce94b42e6..54d4dbe14b38 100644
--- a/compilerplugins/clang/test/refcounting.cxx
+++ b/compilerplugins/clang/test/refcounting.cxx
@@ -107,11 +107,26 @@ void foo7()
UnoSubObject* p3 = static_cast<UnoSubObject*>(getConstRef().get());
(void)p3;
p3 = static_cast<UnoSubObject*>(getConstRef().get());
+}
+
+const unotools::WeakReference<UnoObject>& getWeakRef();
+void foo8()
+{
+ // expected-error@+1 {{weak object being converted to strong, and then the
reference dropped, and managed via raw pointer, should be managed via
rtl::Reference [loplugin:refcounting]}}
+ UnoSubObject* p1 = static_cast<UnoSubObject*>(getWeakRef().get().get());
+ (void)p1;
+
+ // expected-error@+1 {{weak object being converted to strong, and then the
reference dropped, and managed via raw pointer, should be managed via
rtl::Reference [loplugin:refcounting]}}
+ UnoObject* p2 = getWeakRef().get().get();
+ (void)p2;
- // no warning expected, although, arguably, we should be assigning to a
rtl::Reference temporary
unotools::WeakReference<UnoObject> weak1;
- auto pTextObj = dynamic_cast<UnoSubObject*>(weak1.get().get());
- (void)pTextObj;
-}
+ // expected-error@+1 {{weak object being converted to strong, and then the
reference dropped, and managed via raw pointer, should be managed via
rtl::Reference [loplugin:refcounting]}}
+ UnoSubObject* p3 = dynamic_cast<UnoSubObject*>(weak1.get().get());
+ (void)p3;
+ // expected-error@+1 {{weak object being converted to strong, and then the
reference dropped, and managed via raw pointer, should be managed via
rtl::Reference [loplugin:refcounting]}}
+ UnoObject* p4 = weak1.get().get();
+ (void)p4;
+}
/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s
cinkeys+=0=break: */
diff --git a/sd/source/ui/view/Outliner.cxx b/sd/source/ui/view/Outliner.cxx
index 6384f1ee58d7..00e495c8b1d4 100644
--- a/sd/source/ui/view/Outliner.cxx
+++ b/sd/source/ui/view/Outliner.cxx
@@ -1166,7 +1166,7 @@ bool lclIsValidTextObject(const
sd::outliner::IteratorPosition& rPosition)
bool isValidVectorGraphicObject(const sd::outliner::IteratorPosition&
rPosition)
{
- auto* pGraphicObject =
dynamic_cast<SdrGrafObj*>(rPosition.mxObject.get().get());
+ rtl::Reference<SdrGrafObj> pGraphicObject =
dynamic_cast<SdrGrafObj*>(rPosition.mxObject.get().get());
if (pGraphicObject)
{
auto const& pVectorGraphicData =
pGraphicObject->GetGraphic().getVectorGraphicData();
diff --git a/sw/source/uibase/uiview/uivwimp.cxx
b/sw/source/uibase/uiview/uivwimp.cxx
index efe2a713d635..ee9693bd8c3b 100644
--- a/sw/source/uibase/uiview/uivwimp.cxx
+++ b/sw/source/uibase/uiview/uivwimp.cxx
@@ -212,7 +212,7 @@ void SwView_Impl::Invalidate()
GetUNOObject_Impl()->Invalidate();
for (const auto& xTransferable: mxTransferables)
{
- auto pTransferable = xTransferable.get().get();
+ rtl::Reference<SwTransferable> pTransferable = xTransferable.get();
if(pTransferable)
pTransferable->Invalidate();
}
