Bug 100782 - Have XPropertyList and its derivatives hold smart pointers
Summary: Have XPropertyList and its derivatives hold smart pointers
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: All All
: medium normal
Assignee: JoNi
URL:
Whiteboard: target:5.3.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2016-07-06 10:07 UTC by Katarina Behrens (Inactive)
Modified: 2017-02-14 08:57 UTC (History)
1 user (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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 ?