From 582e89610b366c0d887baa6b8de7fa5f065900fa Mon Sep 17 00:00:00 2001 From: Michael Meeks Date: Mon, 30 Mar 2015 17:49:20 +0100 Subject: vclptr: create Instance helpers, and set initial ref-count to 1. Document that in README.lifecycle; the problem is that our constructors currently take and release references left/right on the object being created, which ... means we need an initial reference. Change-Id: I5de952b73ac67888c3fbb150d4a7cde2a7bc9abf --- include/rtl/ref.hxx | 8 +++++- include/vcl/vclptr.hxx | 50 +++++++++++++++++++++++++++++++++++-- vcl/README.lifecycle | 36 +++++++++++++++++++------- vcl/qa/cppunit/lifecycle.cxx | 12 ++++++--- vcl/source/outdev/outdev.cxx | 2 +- vcl/source/window/dialog.cxx | 2 +- vcl/source/window/menubarwindow.cxx | 2 +- vcl/source/window/paint.cxx | 2 +- vcl/source/window/printdlg.cxx | 6 ++--- vcl/source/window/toolbox.cxx | 2 +- vcl/unx/generic/printer/cupsmgr.cxx | 2 +- vcl/unx/x11/x11sys.cxx | 2 +- vcl/workben/outdevgrind.cxx | 2 +- 13 files changed, 101 insertions(+), 27 deletions(-) diff --git a/include/rtl/ref.hxx b/include/rtl/ref.hxx index 942923ad6819..097584b1a276 100644 --- a/include/rtl/ref.hxx +++ b/include/rtl/ref.hxx @@ -47,6 +47,13 @@ public: {} + /** Constructor... + */ + inline Reference (reference_type * pBody, __sal_NoAcquire) + : m_pBody (pBody) + { + } + /** Constructor... */ inline Reference (reference_type * pBody) @@ -56,7 +63,6 @@ public: m_pBody->acquire(); } - /** Copy constructor... */ inline Reference (const Reference & handle) diff --git a/include/vcl/vclptr.hxx b/include/vcl/vclptr.hxx index 6ab6373eaf70..0c29f11d8e2d 100644 --- a/include/vcl/vclptr.hxx +++ b/include/vcl/vclptr.hxx @@ -22,6 +22,7 @@ #include #include +#include /// @cond INTERNAL namespace vcl { namespace detail { @@ -90,14 +91,12 @@ public: : m_rInnerRef() {} - /** Constructor... */ inline VclPtr (reference_type * pBody) : m_rInnerRef(pBody) {} - /** Copy constructor... */ inline VclPtr (const VclPtr & handle) @@ -216,8 +215,31 @@ public: { return (m_rInnerRef > handle.m_rInnerRef); } + +protected: + inline VclPtr (reference_type * pBody, __sal_NoAcquire) + : m_rInnerRef(pBody, SAL_NO_ACQUIRE) + {} }; // class VclPtr +/** + * A construction helper for VclPtr. Since VclPtr types are created + * with a reference-count of one - to help fit into the existing + * code-flow; this helps us to construct them easily. + * + * For more details on the design please see vcl/README.lifecycle + * + * @param reference_type must be a subclass of vcl::Window + */ +template +class VclPtrInstance : public VclPtr +{ +public: + template VclPtrInstance(Arg &&... arg) + : VclPtr( new reference_type(std::forward(arg)...), SAL_NO_ACQUIRE ) + { + } +}; template class ScopedVclPtr : public VclPtr @@ -277,6 +299,30 @@ private: ScopedVclPtr (const ScopedVclPtr &) SAL_DELETED_FUNCTION; // And certainly we don't want a default assignment operator. ScopedVclPtr& SAL_CALL operator= (const ScopedVclPtr &) SAL_DELETED_FUNCTION; + +protected: + inline ScopedVclPtr (reference_type * pBody, __sal_NoAcquire) + : VclPtr(pBody, SAL_NO_ACQUIRE) + {} +}; + +/** + * A construction helper for ScopedVclPtr. Since VclPtr types are created + * with a reference-count of one - to help fit into the existing + * code-flow; this helps us to construct them easily. + * + * For more details on the design please see vcl/README.lifecycle + * + * @param reference_type must be a subclass of vcl::Window + */ +template +class ScopedVclPtrInstance : public ScopedVclPtr +{ +public: + template ScopedVclPtrInstance(Arg &&... arg) + : ScopedVclPtr( new reference_type(std::forward(arg)...), SAL_NO_ACQUIRE ) + { + } }; #endif // INCLUDED_VCL_PTR_HXX diff --git a/vcl/README.lifecycle b/vcl/README.lifecycle index 26e7a348cc39..6a9e080c176a 100644 --- a/vcl/README.lifecycle +++ b/vcl/README.lifecycle @@ -45,8 +45,9 @@ to lingering pointers to freed objects. to reduce code-thrash. VclPtr is used to wrap all OutputDevice derived classes thus: - VclPtr pDialog( new Dialog( ... ) ); - // gotcha - this is not a good idea ... + VclPtr pDialog( new Dialog( ... ), SAL_NO_ACQUIRE ); + ... + pDialog.disposeAndClear(); However - while the VclPtr reference count controls the lifecycle of the Dialog object, it is necessary to be able to @@ -84,30 +85,47 @@ to lingering pointers to freed objects. Luckily it is easy to avoid that with a ScopedVclPtr which does this for you when it goes out of scope. +** One extra gotcha - an initial reference-count of 1 + + In the normal world of love and sanity, eg. creating UNO + objects, the objects start with a ref-count of zero. Thus + the first reference is always taken after construction by + the surrounding smart pointer. + + Unfortunately, the existing VCL code is somewhat tortured, + and does a lot of reference and de-reference action on the + class -during- construction. This forces us to construct with + a reference of 1 - and to hand that into the initial smart + pointer with a SAL_NO_ACQUIRE. + + To make this easier, we have 'Instance' template wrappers + that make this apparently easier, by constructing the + pointer for you. + ** How does my familiar code change ? Lets tweak the exemplary code above to fit the new model: -- Dialog aDialog(...); +- Dialog aDialog(... dialog params ... ); - aDialog.Execute(...); -+ ScopedVclPtr pDialog(new Dialog(...)); ++ ScopedVclPtrInstance pDialog(... dialog params ... ); + pDialog->Execute(...); // VclPtr behaves much like a pointer or: -- Dialog *pDialog = new Dialog(...); -+ VclPtr pDialog(newDialog(...)); +- Dialog *pDialog = new Dialog(... dialog params ...); ++ VclPtrInstance pDialog(... dialog params ...); pDialog->Execute(...); - delete pDialog; + pDialog.disposeAndClear(); // done manually - replaces a delete or: -- boost::shared_ptr xDialog(new pDialog()); -+ ScopedVclPtr xDialog(new Dialog(...)); +- boost::shared_ptr xDialog(new Dialog(...)); ++ ScopedVclPtrInstance xDialog(...); xDialog->Execute(...); + // depending how shared_ptr was shared perhaps + // someone else gets a VclPtr to xDialog or: - VirtualDevice aDev; -+ ScopedVclPtr pDev(new VirtualDevice()); ++ ScopedVclPtrInstance pDev(); ** Why are these 'disposeOnce' calls in destructors ? diff --git a/vcl/qa/cppunit/lifecycle.cxx b/vcl/qa/cppunit/lifecycle.cxx index 12b0a5146631..7c1b1c220f48 100644 --- a/vcl/qa/cppunit/lifecycle.cxx +++ b/vcl/qa/cppunit/lifecycle.cxx @@ -59,7 +59,11 @@ void LifecycleTest::testCast() void LifecycleTest::testVirtualDevice() { - VclPtr pVDev = new VirtualDevice(); + VclPtr pVDev( new VirtualDevice() ); + ScopedVclPtr pVDev2( new VirtualDevice() ); + VclPtrInstance pVDev3; + VclPtrInstance pVDev4( 1 ); + CPPUNIT_ASSERT(!!pVDev && !!pVDev2 && !!pVDev3 && !!pVDev4); } void LifecycleTest::testMultiDispose() @@ -99,8 +103,8 @@ void LifecycleTest::testIsolatedWidgets() void LifecycleTest::testParentedWidgets() { - VclPtr xWin(new WorkWindow((vcl::Window *)NULL, - WB_APP|WB_STDWORK)); + ScopedVclPtrInstance xWin(new WorkWindow((vcl::Window *)NULL, + WB_APP|WB_STDWORK)); CPPUNIT_ASSERT(xWin.get() != NULL); xWin->Show(); testWidgets(xWin); @@ -129,7 +133,7 @@ void LifecycleTest::testChildDispose() void LifecycleTest::testPostDispose() { - VclPtr xWin(new WorkWindow((vcl::Window *)NULL, WB_STDWORK)); + VclPtrInstance xWin((vcl::Window *)NULL, WB_STDWORK); xWin->disposeOnce(); // check selected methods continue to work post-dispose diff --git a/vcl/source/outdev/outdev.cxx b/vcl/source/outdev/outdev.cxx index beee1b7fb63f..e42c8c50bed5 100644 --- a/vcl/source/outdev/outdev.cxx +++ b/vcl/source/outdev/outdev.cxx @@ -82,7 +82,7 @@ namespace { // Begin initializer and accessor public functions OutputDevice::OutputDevice() : - mnRefCnt(0), + mnRefCnt(1), // cf. VclPtrInstance and README.lifecycle maRegion(true), maFillColor( COL_WHITE ), maTextLineColor( COL_TRANSPARENT ), diff --git a/vcl/source/window/dialog.cxx b/vcl/source/window/dialog.cxx index 9ca9c30a1b40..e59ba305a80a 100644 --- a/vcl/source/window/dialog.cxx +++ b/vcl/source/window/dialog.cxx @@ -1065,7 +1065,7 @@ void Dialog::GrabFocusToFirstControl() void Dialog::GetDrawWindowBorder( sal_Int32& rLeftBorder, sal_Int32& rTopBorder, sal_Int32& rRightBorder, sal_Int32& rBottomBorder ) const { - ScopedVclPtr aImplWin( new ImplBorderWindow( (vcl::Window*)this, WB_BORDER|WB_STDWORK, BORDERWINDOW_STYLE_OVERLAP ) ); + ScopedVclPtrInstance aImplWin( (vcl::Window*)this, WB_BORDER|WB_STDWORK, BORDERWINDOW_STYLE_OVERLAP ); aImplWin->GetBorder( rLeftBorder, rTopBorder, rRightBorder, rBottomBorder ); } diff --git a/vcl/source/window/menubarwindow.cxx b/vcl/source/window/menubarwindow.cxx index 0f49c51e78b0..7c0bca777f06 100644 --- a/vcl/source/window/menubarwindow.cxx +++ b/vcl/source/window/menubarwindow.cxx @@ -60,7 +60,7 @@ void DecoToolBox::DataChanged( const DataChangedEvent& rDCEvt ) void DecoToolBox::calcMinSize() { - ScopedVclPtr aTbx( new ToolBox(GetParent()) ); + ScopedVclPtrInstance aTbx( GetParent() ); if( GetItemCount() == 0 ) { ResMgr* pResMgr = ImplGetResMgr(); diff --git a/vcl/source/window/paint.cxx b/vcl/source/window/paint.cxx index 0f05ded46af0..1dd9b3a50e98 100644 --- a/vcl/source/window/paint.cxx +++ b/vcl/source/window/paint.cxx @@ -1094,7 +1094,7 @@ void Window::ImplPaintToDevice( OutputDevice* i_pTargetOutDev, const Point& i_rP mpWindowImpl->mbReallyVisible = bRVisible; // paint metafile to VDev - VclPtr pMaskedDevice = new VirtualDevice( *i_pTargetOutDev, 0, 0 ); + VclPtrInstance pMaskedDevice( *i_pTargetOutDev, 0, 0 ); pMaskedDevice->SetOutputSizePixel( GetOutputSizePixel() ); pMaskedDevice->EnableRTL( IsRTLEnabled() ); aMtf.WindStart(); diff --git a/vcl/source/window/printdlg.cxx b/vcl/source/window/printdlg.cxx index 4ab06b61049c..f173ad36915d 100644 --- a/vcl/source/window/printdlg.cxx +++ b/vcl/source/window/printdlg.cxx @@ -624,13 +624,13 @@ PrintDialog::PrintDialog( vcl::Window* i_pParent, const std::shared_ptrGetEntryPos( aValue ) != LISTBOX_ENTRY_NOTFOUND ) { maJobPage.mpPrinters->SelectEntry( aValue ); - maPController->setPrinter( VclPtr( new Printer( aValue ) ) ); + maPController->setPrinter( VclPtrInstance( aValue ) ); } else { // fall back to default printer maJobPage.mpPrinters->SelectEntry( Printer::GetDefaultPrinterName() ); - maPController->setPrinter( VclPtr( new Printer( Printer::GetDefaultPrinterName() ) ) ); + maPController->setPrinter( VclPtrInstance( Printer::GetDefaultPrinterName() ) ); } } // not printing to file @@ -1521,7 +1521,7 @@ IMPL_LINK( PrintDialog, SelectHdl, ListBox*, pBox ) { OUString aNewPrinter( pBox->GetSelectEntry() ); // set new printer - maPController->setPrinter( VclPtr( new Printer( aNewPrinter ) ) ); + maPController->setPrinter( VclPtrInstance( aNewPrinter ) ); maPController->resetPrinterOptions( maOptionsPage.mpToFileBox->IsChecked() ); // update text fields updatePrinterText(); diff --git a/vcl/source/window/toolbox.cxx b/vcl/source/window/toolbox.cxx index 58ec9241039a..b434061fb274 100644 --- a/vcl/source/window/toolbox.cxx +++ b/vcl/source/window/toolbox.cxx @@ -4833,7 +4833,7 @@ Size ToolBox::CalcMinimumWindowSizePixel() const else { // create dummy toolbox for measurements - VclPtr< ToolBox > pToolBox = new ToolBox( GetParent(), GetStyle() ); + VclPtrInstance< ToolBox > pToolBox( GetParent(), GetStyle() ); // copy until first useful item std::vector< ImplToolItem >::iterator it = mpData->m_aItems.begin(); diff --git a/vcl/unx/generic/printer/cupsmgr.cxx b/vcl/unx/generic/printer/cupsmgr.cxx index daddd1ac286f..d51f752463df 100644 --- a/vcl/unx/generic/printer/cupsmgr.cxx +++ b/vcl/unx/generic/printer/cupsmgr.cxx @@ -966,7 +966,7 @@ namespace { bool bRet = false; - ScopedVclPtr aDialog(new RTSPWDialog(rServer, rUserName, NULL)); + ScopedVclPtrInstance aDialog(rServer, rUserName, nullptr); if (aDialog->Execute()) { rUserName = aDialog->getUserName(); diff --git a/vcl/unx/x11/x11sys.cxx b/vcl/unx/x11/x11sys.cxx index 8cad695b5265..6723c63b8047 100644 --- a/vcl/unx/x11/x11sys.cxx +++ b/vcl/unx/x11/x11sys.cxx @@ -134,7 +134,7 @@ int X11SalSystem::ShowNativeDialog( const OUString& rTitle, const OUString& rMes if( pSVData->mpIntroWindow ) pSVData->mpIntroWindow->Hide(); - ScopedVclPtr aWarn(new WarningBox(NULL, WB_STDWORK, rMessage) ); + ScopedVclPtrInstance aWarn(nullptr, WB_STDWORK, rMessage); aWarn->SetText( rTitle ); aWarn->Clear(); diff --git a/vcl/workben/outdevgrind.cxx b/vcl/workben/outdevgrind.cxx index d53074847f90..da14e1aac673 100644 --- a/vcl/workben/outdevgrind.cxx +++ b/vcl/workben/outdevgrind.cxx @@ -898,7 +898,7 @@ sal_uInt16 GrindApp::Exception( sal_uInt16 nError ) int GrindApp::Main() { - ScopedVclPtr aWindow(new TestWindow); + ScopedVclPtrInstance aWindow; aWindow->Execute(); return 0; } -- cgit