[Libreoffice-commits] core.git: accessibility/source canvas/source chart2/source compilerplugins/clang sc/source sd/source writerfilter/source
Noel (via logerrit)
logerrit at kemper.freedesktop.org
Mon Mar 8 06:38:10 UTC 2021
accessibility/source/standard/vclxaccessibletoolbox.cxx | 4
canvas/source/opengl/ogl_canvashelper.cxx | 3
chart2/source/controller/main/ChartController_Window.cxx | 6 -
compilerplugins/clang/refcounting.cxx | 72 +++++++++++++--
compilerplugins/clang/test/refcounting.cxx | 22 ++++
sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx | 3
sd/source/ui/dlg/sdtreelb.cxx | 3
writerfilter/source/ooxml/OOXMLFastHelper.hxx | 6 -
8 files changed, 104 insertions(+), 15 deletions(-)
New commits:
commit 04e7a34a19b3658de57c4f2b3b0fa8445b01f199
Author: Noel <noel.grandin at collabora.co.uk>
AuthorDate: Fri Mar 5 15:51:07 2021 +0200
Commit: Noel Grandin <noel.grandin at collabora.co.uk>
CommitDate: Mon Mar 8 07:37:26 2021 +0100
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 <noel.grandin at collabora.co.uk>
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<ToolBox *>(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<CanvasFont*>(xLayoutetText->getFont().get());
+ auto pLayoutFont = xLayoutetText->getFont();
+ CanvasFont* pFont=dynamic_cast<CanvasFont*>(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<ChartModel*>(getModel().get());
+ css::uno::Reference< css::frame::XModel > xModel = getModel();
+ ChartModel* pModel = dynamic_cast<ChartModel*>(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<PopupRequest*>(pChartModel->getPopupRequest().get());
+ css::uno::Reference<css::awt::XRequestCallback> xPopupRequest = pChartModel->getPopupRequest();
+ PopupRequest* pPopupRequest = dynamic_cast<PopupRequest*>(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<FooChild*>(makeFoo().get());
+ where makeFoo() returns a Reference<Foo>
+*/
+bool RefCounting::isCastingReference(const Expr* expr)
+{
+ expr = compat::IgnoreImplicit(expr);
+ auto castExpr = dyn_cast<CastExpr>(expr);
+ if (!castExpr)
+ return false;
+ auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(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<DeclRefExpr>(obj) || isa<MemberExpr>(obj))
+ return false;
+ // if the foo in foo().get() returns "rtl::Reference<T>&" 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<CallExpr>(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<CXXNewExpr>(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<UnoObject> foo6()
// no warning expected
return new UnoObject;
}
+const rtl::Reference<UnoObject>& getConstRef();
+void foo7()
+{
+ // expected-error at +1 {{cppu::OWeakObject subclass 'UnoSubObject' being managed via raw pointer, should be managed via rtl::Reference [loplugin:refcounting]}}
+ UnoSubObject* p1 = static_cast<UnoSubObject*>(foo6().get());
+ (void)p1;
+ // expected-error at +1 {{cppu::OWeakObject subclass 'UnoSubObject' being managed via raw pointer, should be managed via rtl::Reference [loplugin:refcounting]}}
+ p1 = static_cast<UnoSubObject*>(foo6().get());
+
+ rtl::Reference<UnoObject> u2;
+ // no warning expected
+ UnoSubObject* p2 = static_cast<UnoSubObject*>(u2.get());
+ (void)p2;
+ p2 = static_cast<UnoSubObject*>(u2.get());
+ // no warning expected
+ UnoSubObject* p3 = static_cast<UnoSubObject*>(getConstRef().get());
+ (void)p3;
+ p3 = static_cast<UnoSubObject*>(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<ScAccessibleDocument*>(getAccessibleParent().get());
+ static_cast<ScAccessibleDocument*>(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<sd::DrawController*>(xFrame->getController().get());
+ auto xController = xFrame->getController();
+ ::sd::DrawController* pController = dynamic_cast<sd::DrawController*>(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 T>
class OOXMLFastHelper
{
public:
- static OOXMLFastContextHandler* createAndSetParentAndDefine
+ static rtl::Reference<OOXMLFastContextHandler> createAndSetParentAndDefine
(OOXMLFastContextHandler * pHandler, sal_uInt32 nToken, Id nId, Id nDefine);
static void newProperty(OOXMLFastContextHandler * pHandler,
@@ -36,9 +36,9 @@ public:
};
template <class T>
-OOXMLFastContextHandler* OOXMLFastHelper<T>::createAndSetParentAndDefine (OOXMLFastContextHandler * pHandler, sal_uInt32 nToken, Id nId, Id nDefine)
+rtl::Reference<OOXMLFastContextHandler> OOXMLFastHelper<T>::createAndSetParentAndDefine (OOXMLFastContextHandler * pHandler, sal_uInt32 nToken, Id nId, Id nDefine)
{
- OOXMLFastContextHandler * pTmp = new T(pHandler);
+ rtl::Reference<OOXMLFastContextHandler> pTmp = new T(pHandler);
pTmp->setToken(nToken);
pTmp->setId(nId);
More information about the Libreoffice-commits
mailing list