From 04e7a34a19b3658de57c4f2b3b0fa8445b01f199 Mon Sep 17 00:00:00 2001 From: Noel Date: Fri, 5 Mar 2021 15:51:07 +0200 Subject: [PATCH] loplugin:refcounting check for one more case where we might be holding something newly created by pointer instead of by *::Reference Change-Id: Ife6f7acae4252bf56dcdeb95d72e43c523444f97 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/112138 Tested-by: Jenkins Reviewed-by: Noel Grandin --- .../source/standard/vclxaccessibletoolbox.cxx | 4 +- canvas/source/opengl/ogl_canvashelper.cxx | 3 +- .../main/ChartController_Window.cxx | 6 +- compilerplugins/clang/refcounting.cxx | 72 +++++++++++++++++-- compilerplugins/clang/test/refcounting.cxx | 22 ++++++ .../Accessibility/AccessibleSpreadsheet.cxx | 3 +- sd/source/ui/dlg/sdtreelb.cxx | 3 +- writerfilter/source/ooxml/OOXMLFastHelper.hxx | 6 +- 8 files changed, 104 insertions(+), 15 deletions(-) diff --git a/accessibility/source/standard/vclxaccessibletoolbox.cxx b/accessibility/source/standard/vclxaccessibletoolbox.cxx index acd2c4f18899..a04685730c85 100644 --- a/accessibility/source/standard/vclxaccessibletoolbox.cxx +++ b/accessibility/source/standard/vclxaccessibletoolbox.cxx @@ -593,8 +593,8 @@ void VCLXAccessibleToolBox::ProcessWindowEvent( const VclWindowEvent& rVclWindow if ( pWin && pWin->GetParent() && pWin->GetParent()->GetType() == WindowType::TOOLBOX ) { - VCLXAccessibleToolBox* pParent = static_cast< VCLXAccessibleToolBox* >( - pWin->GetParent()->GetAccessible()->getAccessibleContext().get() ); + auto pParentAccContext = pWin->GetParent()->GetAccessible()->getAccessibleContext(); + VCLXAccessibleToolBox* pParent = static_cast< VCLXAccessibleToolBox* >( pParentAccContext.get() ); if ( pParent ) pParent->ReleaseSubToolBox(static_cast(pWin.get())); } diff --git a/canvas/source/opengl/ogl_canvashelper.cxx b/canvas/source/opengl/ogl_canvashelper.cxx index d64e1ba1d7f0..0484f710ae53 100644 --- a/canvas/source/opengl/ogl_canvashelper.cxx +++ b/canvas/source/opengl/ogl_canvashelper.cxx @@ -679,7 +679,8 @@ namespace oglcanvas ScopedVclPtrInstance< VirtualDevice > pVDev; pVDev->EnableOutput(false); - CanvasFont* pFont=dynamic_cast(xLayoutetText->getFont().get()); + auto pLayoutFont = xLayoutetText->getFont(); + CanvasFont* pFont=dynamic_cast(pLayoutFont.get()); const rendering::StringContext& rTxt=xLayoutetText->getText(); if( pFont && rTxt.Length ) { diff --git a/chart2/source/controller/main/ChartController_Window.cxx b/chart2/source/controller/main/ChartController_Window.cxx index a8d71e903fd3..afd4de8a5e75 100644 --- a/chart2/source/controller/main/ChartController_Window.cxx +++ b/chart2/source/controller/main/ChartController_Window.cxx @@ -863,7 +863,8 @@ void ChartController::execute_MouseButtonUp( const MouseEvent& rMEvt ) m_xUndoManager ); bool bChanged = false; - ChartModel* pModel = dynamic_cast(getModel().get()); + css::uno::Reference< css::frame::XModel > xModel = getModel(); + ChartModel* pModel = dynamic_cast(xModel.get()); assert(pModel); if ( eObjectType == OBJECTTYPE_LEGEND ) bChanged = DiagramHelper::switchDiagramPositioningToExcludingPositioning( *pModel, false , true ); @@ -2090,7 +2091,8 @@ void ChartController::sendPopupRequest(OUString const & rCID, tools::Rectangle a OUString sPivotTableName = xPivotTableDataProvider->getPivotTableName(); - PopupRequest* pPopupRequest = dynamic_cast(pChartModel->getPopupRequest().get()); + css::uno::Reference xPopupRequest = pChartModel->getPopupRequest(); + PopupRequest* pPopupRequest = dynamic_cast(xPopupRequest.get()); if (!pPopupRequest) return; diff --git a/compilerplugins/clang/refcounting.cxx b/compilerplugins/clang/refcounting.cxx index 319a23b83a9b..9157a1910add 100644 --- a/compilerplugins/clang/refcounting.cxx +++ b/compilerplugins/clang/refcounting.cxx @@ -73,6 +73,7 @@ private: const RecordDecl* parent, const std::string& rDeclName); bool visitTemporaryObjectExpr(Expr const * expr); + bool isCastingReference(const Expr* expr); }; bool containsXInterfaceSubclass(const clang::Type* pType0); @@ -693,6 +694,56 @@ bool RefCounting::VisitVarDecl(const VarDecl * varDecl) { << pointeeType << varDecl->getSourceRange(); } + if (isCastingReference(varDecl->getInit())) + { + // TODO false+ code + StringRef fileName = getFilenameOfLocation(compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(varDecl))); + if (loplugin::isSamePathname(fileName, SRCDIR "/sw/source/core/unocore/unotbl.cxx")) + return true; + auto pointeeType = varDecl->getType()->getPointeeType(); + if (containsOWeakObjectSubclass(pointeeType)) + report( + DiagnosticsEngine::Warning, + "cppu::OWeakObject subclass %0 being managed via raw pointer, should be managed via rtl::Reference", + varDecl->getLocation()) + << pointeeType + << varDecl->getSourceRange(); + } + } + return true; +} + +/** + Look for code like + static_cast(makeFoo().get()); + where makeFoo() returns a Reference +*/ +bool RefCounting::isCastingReference(const Expr* expr) +{ + expr = compat::IgnoreImplicit(expr); + auto castExpr = dyn_cast(expr); + if (!castExpr) + return false; + auto memberCallExpr = dyn_cast(castExpr->getSubExpr()); + if (!memberCallExpr) + return false; + if (!memberCallExpr->getMethodDecl()->getIdentifier() || memberCallExpr->getMethodDecl()->getName() != "get") + return false; + QualType objectType = memberCallExpr->getImplicitObjectArgument()->getType(); + if (!loplugin::TypeCheck(objectType).Class("Reference")) + return false; + // ignore "x.get()" where x is a var + auto obj = compat::IgnoreImplicit(memberCallExpr->getImplicitObjectArgument()); + if (isa(obj) || isa(obj)) + return false; + // if the foo in foo().get() returns "rtl::Reference&" then the variable + // we are assigning to does not __have__ to be Reference, since the method called + // must already be holding a reference. + if (auto callExpr = dyn_cast(obj)) + { + if (auto callMethod = callExpr->getDirectCallee()) + if (callMethod->getReturnType()->isReferenceType()) + return false; } return true; } @@ -706,14 +757,14 @@ bool RefCounting::VisitBinaryOperator(const BinaryOperator * binaryOperator) if (!binaryOperator->getLHS()->getType()->isPointerType()) return true; - // deliberately does not want to keep track at the allocation site - StringRef fileName = getFilenameOfLocation(compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(binaryOperator))); - if (loplugin::isSamePathname(fileName, SRCDIR "/vcl/unx/generic/dtrans/X11_selection.cxx")) - return true; - auto newExpr = dyn_cast(compat::IgnoreImplicit(binaryOperator->getRHS())); if (newExpr) { + // deliberately does not want to keep track at the allocation site + StringRef fileName = getFilenameOfLocation(compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(binaryOperator))); + if (loplugin::isSamePathname(fileName, SRCDIR "/vcl/unx/generic/dtrans/X11_selection.cxx")) + return true; + auto pointeeType = binaryOperator->getLHS()->getType()->getPointeeType(); if (containsOWeakObjectSubclass(pointeeType)) { @@ -725,6 +776,17 @@ bool RefCounting::VisitBinaryOperator(const BinaryOperator * binaryOperator) << binaryOperator->getSourceRange(); } } + if (isCastingReference(binaryOperator->getRHS())) + { + auto pointeeType = binaryOperator->getLHS()->getType()->getPointeeType(); + if (containsOWeakObjectSubclass(pointeeType)) + report( + DiagnosticsEngine::Warning, + "cppu::OWeakObject subclass %0 being managed via raw pointer, should be managed via rtl::Reference", + compat::getBeginLoc(binaryOperator)) + << pointeeType + << binaryOperator->getSourceRange(); + } return true; } diff --git a/compilerplugins/clang/test/refcounting.cxx b/compilerplugins/clang/test/refcounting.cxx index 4c133023b0b8..7ab830fc913b 100644 --- a/compilerplugins/clang/test/refcounting.cxx +++ b/compilerplugins/clang/test/refcounting.cxx @@ -27,6 +27,9 @@ public: struct UnoObject : public cppu::OWeakObject { }; +struct UnoSubObject : public UnoObject +{ +}; // // Note, getting duplicate warnings for some reason I cannot fathom @@ -94,5 +97,24 @@ rtl::Reference foo6() // no warning expected return new UnoObject; } +const rtl::Reference& getConstRef(); +void foo7() +{ + // expected-error@+1 {{cppu::OWeakObject subclass 'UnoSubObject' being managed via raw pointer, should be managed via rtl::Reference [loplugin:refcounting]}} + UnoSubObject* p1 = static_cast(foo6().get()); + (void)p1; + // expected-error@+1 {{cppu::OWeakObject subclass 'UnoSubObject' being managed via raw pointer, should be managed via rtl::Reference [loplugin:refcounting]}} + p1 = static_cast(foo6().get()); + + rtl::Reference u2; + // no warning expected + UnoSubObject* p2 = static_cast(u2.get()); + (void)p2; + p2 = static_cast(u2.get()); + // no warning expected + UnoSubObject* p3 = static_cast(getConstRef().get()); + (void)p3; + p3 = static_cast(getConstRef().get()); +} /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ diff --git a/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx b/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx index aee01834103e..457adf6d0213 100644 --- a/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx +++ b/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx @@ -493,8 +493,9 @@ void ScAccessibleSpreadsheet::Notify( SfxBroadcaster& rBC, const SfxHint& rHint if(aNewCell.Tab() != maActiveCell.Tab()) { aEvent.EventId = AccessibleEventId::PAGE_CHANGED; + auto pAccParent = getAccessibleParent(); ScAccessibleDocument *pAccDoc = - static_cast(getAccessibleParent().get()); + static_cast(pAccParent.get()); if(pAccDoc) { pAccDoc->CommitChange(aEvent); diff --git a/sd/source/ui/dlg/sdtreelb.cxx b/sd/source/ui/dlg/sdtreelb.cxx index 764301858ad4..1afad1a87d48 100644 --- a/sd/source/ui/dlg/sdtreelb.cxx +++ b/sd/source/ui/dlg/sdtreelb.cxx @@ -612,7 +612,8 @@ void SdPageObjsTLV::AddShapeToTransferable ( if ( ! (xFrameAccess->getByIndex(nIndex) >>= xFrame)) continue; - ::sd::DrawController* pController = dynamic_cast(xFrame->getController().get()); + auto xController = xFrame->getController(); + ::sd::DrawController* pController = dynamic_cast(xController.get()); if (pController == nullptr) continue; ::sd::ViewShellBase* pBase = pController->GetViewShellBase(); diff --git a/writerfilter/source/ooxml/OOXMLFastHelper.hxx b/writerfilter/source/ooxml/OOXMLFastHelper.hxx index b68baee63b96..89bbf0c850b6 100644 --- a/writerfilter/source/ooxml/OOXMLFastHelper.hxx +++ b/writerfilter/source/ooxml/OOXMLFastHelper.hxx @@ -28,7 +28,7 @@ template class OOXMLFastHelper { public: - static OOXMLFastContextHandler* createAndSetParentAndDefine + static rtl::Reference createAndSetParentAndDefine (OOXMLFastContextHandler * pHandler, sal_uInt32 nToken, Id nId, Id nDefine); static void newProperty(OOXMLFastContextHandler * pHandler, @@ -36,9 +36,9 @@ public: }; template -OOXMLFastContextHandler* OOXMLFastHelper::createAndSetParentAndDefine (OOXMLFastContextHandler * pHandler, sal_uInt32 nToken, Id nId, Id nDefine) +rtl::Reference OOXMLFastHelper::createAndSetParentAndDefine (OOXMLFastContextHandler * pHandler, sal_uInt32 nToken, Id nId, Id nDefine) { - OOXMLFastContextHandler * pTmp = new T(pHandler); + rtl::Reference pTmp = new T(pHandler); pTmp->setToken(nToken); pTmp->setId(nId);