Bug 34465 - get rid of all calls to virtual const SfxPoolItem* Put( const SfxPoolItem&, USHORT nWhich)
Summary: get rid of all calls to virtual const SfxPoolItem* Put( const SfxPoolItem&, U...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium enhancement
Assignee: Not Assigned
URL:
Whiteboard: target:5.3.0
Keywords: difficultyInteresting, skillCpp
Depends on:
Blocks: 39849
  Show dependency treegraph
 
Reported: 2011-02-18 15:31 UTC by Björn Michaelsen
Modified: 2017-11-05 22:01 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
Patches (20.39 KB, application/x-gzip)
2013-07-23 07:16 UTC, Maciej Rumianowski
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Michaelsen 2011-02-18 15:31:26 UTC
SfxItemSet provides a API that is inviting errors. SfxPoolItems store a "Which-Id" which can be thought of as a primitive kind of type information. Items are stored in a SfxItemSet, which keeps one or zero items of a specific "Which-Id". However, it also provides a method: virtual const SfxPoolItem* Put( const SfxPoolItem&, USHORT nWhich ); which allows putting a SfxPoolItem at a different Which-Id as the one it was created with. This contradicts developer expectations and also blocks further basic refactoring of the SfxItemSets, which have a huge part in the load/save performance and thus are a valuable target. Here is an random example of a client that abuses the SfxItemSet::Put() method: it gets the item from one Which-Id and puts it at explicitly at multiple locations in another (causing the Which-Id of the Item and the Which-Id where it is recorded in the set to differ). Instead it should create copies of the item with the new Which-Id and call const SfxPoolItem* Put( const SfxPoolItem& rItem ). Once all the call to virtual const SfxPoolItem* Put( const SfxPoolItem&, USHORT nWhich ); are gone, it should be immediately removed. Then SfxItemSet can be refactored allowing better performance and debugging and less developer confusion.

I tried to do this in cws new_itemsets on OOo, but the cws is outdated and still incomplete. I started with the reimplementation of the itemsets, which was the wrong point to start: Instead one should:

1) the removal of the bad calls which put items at WhichIds were they do not belong
2) the removal of all other call to virtual const SfxPoolItem* Put( const SfxPoolItem&, USHORT nWhich), by replacing them with call with calls to const SfxPoolItem* Put(const SfxPoolItem& rItem) 
   CAUTION: This cannot be done before step one is completed, as Items are often pulled out of one set and put into another at the same WhichId as it was in the original set. If the WhichId on the item and were it was in the set differ, this is bound to break.
3) Remove virtual const SfxPoolItem* Put( const SfxPoolItem&, USHORT nWhich)
4) Replace SfxItemSet implementation with a faster one.
Comment 1 Björn Michaelsen 2011-07-01 06:00:57 UTC
Comments from the EasyHacks wiki:

SfxItemSet provides a API that is inviting errors. SfxPoolItems store a "Which-Id" which can be thought of as a primitive kind of type information. Items are stored in a SfxItemSet, which keeps one or zero items of a specific "Which-Id". However, it also provides a method: [http://opengrok.libreoffice.org/xref/libs-gui/svl/inc/svl/itemset.hxx#146 virtual const SfxPoolItem* Put( const SfxPoolItem&, USHORT nWhich );] which allows putting a SfxPoolItem at a ''different'' Which-Id as the one it was created with. This contradicts developer expectations and also blocks further basic refactoring of the SfxItemSets, which have a huge part in the load/save performance and thus are a valuable target. Here is an random [http://opengrok.libreoffice.org/xref/calc/sc/source/ui/app/inputwin.cxx#974 example of a client] that abuses the SfxItemSet::Put() method: it gets the item from one Which-Id and puts it at explicitly at multiple locations in another (causing the Which-Id of the Item and the Which-Id where it is recorded in the set to differ). Instead it should create copies of the item with the new Which-Id and call [http://opengrok.libreoffice.org/xref/libs-gui/svl/inc/svl/itemset.hxx#147 const SfxPoolItem* Put( const SfxPoolItem& rItem )]. Once all the call to [http://opengrok.libreoffice.org/xref/libs-gui/svl/inc/svl/itemset.hxx#146 virtual const SfxPoolItem* Put( const SfxPoolItem&, USHORT nWhich );] are gone, it should be immediately removed. Then SfxItemSet can be refactored allowing better performance and debugging and less developer confusion.
Comment 2 Björn Michaelsen 2011-12-23 11:44:55 UTC Comment hidden (obsolete)
Comment 3 Björn Michaelsen 2011-12-23 12:56:58 UTC
An EasyHack should have been checked by developers and thus is confirmed regardless of age. Moving back to NEW from NEEDINFO again. Sorry for the hassle.
Comment 4 Michael Stahl (allotropia) 2012-04-04 11:56:36 UTC
CC:ing the author of this and myself
Comment 5 Florian Reisinger 2012-05-18 09:40:07 UTC
Deleted "Easyhack" from summary.
Comment 6 Mathias Hasselmann 2013-01-22 14:38:08 UTC
this one blocks bug 39849 if i read things correctly
Comment 7 Maciej Rumianowski 2013-07-22 09:58:41 UTC
I am not working on this. Some info can be found on mailing list. I have some patches but they are not finished.
Comment 8 Julien Nabet 2013-07-22 21:05:10 UTC
Maciej: I read about http://nabble.documentfoundation.org/Bug-34465-get-rid-of-all-calls-to-virtual-const-SfxPoolItem-Put-const-SfxPoolItem-amp-USHORT-nWhich-td4017320.html. Perhaps you could attach your current patches so someone may keep on this work? (I'm not speaking about me since I'm not smart enough to do it :) )
Comment 9 Maciej Rumianowski 2013-07-23 07:16:39 UTC
Created attachment 82856 [details]
Patches

Attaching my patches which are a work in progress and should not be committed to master. I am not sure about last two patches, they break things.
Here is the list of patches:
0001-Add-CloneAtWhich.patch
0002-changes-in-svx.patch
0003-sc-Replace-two-arg-Put-method-with-one-arg-and-clone.patch
0004-changes-in-svl.patch
0005-sw-Replace-two-arg-Put-method-with-one-arg-and-clone.patch
0006-cui-Replace-two-arg-Put-method-with-one-arg-and-clon.patch
0007-sd-Replace-two-arg-Put-method-with-one-arg-and-clone.patch
0008-editeng-Replace-two-arg-Put-method-with-one-arg-and-.patch
0009-Use-one-arg-Put-method-instead.patch
0010-svl-Replace-two-arg-Put-method-with-one-arg-and-clon.patch
0011-remove-Put-const-SfxPoolItem-sal_uInt16-nWhich.patch
0012-correct-handling-of-disabled-items.patch

Master was at eee2fe2e7efe1476d363bfb36b09d7e0d4042438 back then

Hope that helps Someone.
Comment 10 Björn Michaelsen 2013-10-04 18:47:46 UTC
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 11 Björn Michaelsen 2015-01-15 16:25:33 UTC
SfxItemSet is rather nontrivial ... => UnEasyHackify
Comment 12 Robinson Tryon (qubit) 2015-12-14 04:57:38 UTC Comment hidden (obsolete)
Comment 13 Commit Notification 2016-05-27 07:22:44 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9eb2e683ab051edd0bce18841f0ac05df5038854

tdf#34465 remove calls to SfxItemSet::Put(const SfxPoolItem&, sal_uInt16)

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 14 Xisco Faulí 2016-09-15 22:38:30 UTC Comment hidden (obsolete)
Comment 15 Xisco Faulí 2017-11-05 22:01:05 UTC
A polite ping to Noel Grandin: is this bug fixed? if so, could you please close it as RESOLVED FIXED ? Thanks