Bug 100782

Summary:	Have XPropertyList and its derivatives hold smart pointers
Product:	LibreOffice	Reporter:	Katarina Behrens (Inactive) <Katarina.Behrens>
Component:	LibreOffice	Assignee:	JoNi <j.nitschke+tdfbz>
Status:	RESOLVED FIXED
Severity:	normal	CC:	mentoring
Priority:	medium	Keywords:	difficultyBeginner, easyHack, skillCpp, topicCleanup
Version:	Inherited From OOo
Hardware:	All
OS:	All
Whiteboard:	target:5.3.0
Crash report or crash signature:		Regression By:

Description Katarina Behrens (Inactive) 2016-07-06 10:07:18 UTC

XPropertyList and its derivatives (XHatchList, XGradientList, XColorList etc.) hold their items in a vector of raw pointers. Methods such as Replace, Remove return the pointer to the item and it is up to the caller to delete the item, which results in butt-ugly code e.g. like this:

XColorEntry* pEntry = pColorList->Remove( nPos );
assert( pEntry && "ColorEntry not found !" );
delete pEntry;

Or this:

XColorEntry* pEntry = new XColorEntry(aCurrentColor, aName);
delete pColorList->Replace(pEntry, nPos);

This hack is about replacing raw pointers with smart pointers in those containers (and of course adapt the code using them).

Code pointers: start from include/svx/xtable.hxx, svx/source/xoutdev/xtable.cxx. Replace std::vector< XPropertyEntry*>  with std::vector< std::unique_ptr > ( or maybe shared_ptr ... I didn't check all the usage of this class ). 

Adapt users of those lists to use smart instead of raw pointers, compile, rinse and repeat.

Comment 1 Katarina Behrens (Inactive) 2016-07-06 10:07:52 UTC

New as this is an easy hack

Comment 2 Nadith Malinda 2016-07-29 07:40:23 UTC

can you explain more on this bug I would like fix this bug

Comment 3 Katarina Behrens (Inactive) 2016-08-08 09:24:50 UTC

Well, I already outlined something in my initial comment, if it ain't enough, here are some more details

First off, in XPropertyList class (include/svx/xtable.hxx) change XPropertyEntryList_impl typedef to a vector of std::unique_ptrs to XPropertyEntry (instead of raw XPropertyEntry* pointers)

Then, you have to adapt XPropertyEntry::Insert, ::Remove, ::Replace etc. methods (from svx/source/xoutdev/xtable.cxx) to operate on smart pointers instead of raw pointers (you'll need to use reset(), get() etc. and make sure you preserve the semantics of operations when doing so)

Now of course the code using those methods will not compile, which makes it easy to find it (there's lot of users ot XPropertyList and derivatives in cui/source/tabpages, but elsewhere too). Change this code to use the new Insert, Remove, Replace etc. smart pointer-aware methods (that means in most of the cases getting rid of manual delete calls and doing some more tweaks, if applicable). 

Repeat until all the code compiles and all the unit tests pass

Comment 4 jani 2016-08-08 09:33:15 UTC

For a general introduction on how to submit patches, compile LO etc, please have a look at

https://wiki.documentfoundation.org/Development/GetInvolved

Looking forward to see your patches.

Comment 5 Commit Notification 2016-08-15 07:50:24 UTC

Jochen Nitschke committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=2b68e03348b3b4009e8bb2af7979de36bd3450c5

tdf#100782 have XPropertyList hold unique_ptr

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

Comment 6 jani 2016-08-16 06:55:55 UTC

Bugs seems closed ?