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(makeFoo().get().get()); + or + foo.get(); + where makeFoo() returns a unotools::WeakReference + and foo is a unotools::WeakReference var. +*/ +bool RefCounting::isCallingGetOnWeakRef(const Expr* expr) +{ + expr = expr->IgnoreImplicit(); + // unwrap the cast (if any) + if (auto castExpr = dyn_cast(expr)) + expr = castExpr->getSubExpr()->IgnoreImplicit(); + // unwrap outer get (if any) + if (auto memberCallExpr = dyn_cast(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(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(getConstRef().get()); (void)p3; p3 = static_cast(getConstRef().get()); - - // no warning expected, although, arguably, we should be assigning to a rtl::Reference temporary - unotools::WeakReference weak1; - auto pTextObj = dynamic_cast(weak1.get().get()); - (void)pTextObj; } +const unotools::WeakReference& 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(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; + + unotools::WeakReference weak1; + // 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(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(rPosition.mxObject.get().get()); + rtl::Reference pGraphicObject = dynamic_cast(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 pTransferable = xTransferable.get(); if(pTransferable) pTransferable->Invalidate(); }