ITEM: preparations for more/easier changes II

Again this is a change to improve understandabilty/changeability
of SfxItemSet code plus some cleanups. Still did a callgrind round
to check - it slightly got faster. In a start/load(complex SW doc)/
show/shutdown cycle compared with master I get 96.722 mio cycles
compared with 99.851 mio in master.

Main focus was to isolate two aspects: preparation and cleanup of
an Item for usage in an SfxItemSet. For that we now have
implCreateItemEntry: to do all needed actions to create/prepare
an Item for membership, including evtl. AddRef/Cloning/using
ItemPool stuff.
implCleanupItemEntry: to do all needed actios to correctly
clean that Item up again.
All formally accesses distributed over SfxItemSet (and other places)
are cleaned-up to use these. The Item-counter in DBG code that I
already added helped a lot to do this.

Also cleaned up PutImpl to 1st check if action is necessary (Item is
already in place) or not, reducing spot to cleanup an Item that
was handed over as bPassingOwnership to one place.

I also added a 2nd flag, bItemIsSetMember, that tells if the Item
given as input is already member of an SfxItemSet, in that case a
shortcut can be used (increase AddRef, use).

Adapted all places AFAP to use the new container interface
(begin(), end(), ...) where useful.
Made GetItemState inline and directly use the tooling method. Same
for InvalidateItem.
Added much more comments to describe what's going on or to hint at
problems (check for CAUTION).
Removed PutDirect - not needed anymore, probably was there to not get
recursive death loop with callbacks in SW.
More smaller changes.

Checked with all apps, played around. Still, stuff may come up, so
I put on gerrit  the tests will show and give further hints. At last
SfxItemSet is a minefield :-)

Had to adapt SfxItemSet::implCreateItemEntry when input Item is
a StaticDefaultItem. SfxItemPool::PutImpl needs to be called in
that case.

Had to correct bItemIsSetMember in all cases if the transfer
of SfxPoolItems is between SfxItemSets with different
SfxItemPools. This is and will be necessary as long as the
Items are stored at the pool.

After a hard deep-dive I found the error: m_nCount was not in
all cases correct, invalid items get counted.

Win build *insists* for initialzation of local var aEntry in
SfxItemSet::PutImpl, triggers warning C4701:
"potentially uninitialized local variable 'aEntry' used". This
is not the case here, but I know of no way to silence the
warning in another way, so added an extra-call to begin().

Re-added to use static pool defaults directly, possible After
the fix 6d8c6e8d60956fd36094035a526c1a29a902204b, thanks for
that. This avoids some cloning of Items.

CAUTION: static default items are not *that* static as it seems
(or: should be). If they are freed with the Pool (see
::ReleaseDefaults) they will be deleted. If the target pool is
different from the source pool static default items from the
source pool can be used that then might be deleted (sigh).

A solution would be to change all pools to really work on
static instances of default items.

Another one would be to know here that the
targetPool != sourcePool, so maybe extend
bItemIsSetMember -> bSamePool.

A good test for this is CppunitTest_chart2_uichart/testTdf98690.
Until solving/cleaning up we unfortunately *have* to continue
to clone static default items...

Change-Id: Ibd8dc6d612f594a5ad88c75fcee8726d89a6090c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/156306
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>

1 files changed, 27 insertions, 17 deletions

diff --git a/include/svl/itemset.hxx b/include/svl/itemset.hxx
index 37141a0a79b3..ba14c21b8e6c 100644
--- a/include/svl/itemset.hxx
+++ b/include/svl/itemset.hxx
@@ -85,9 +85,7 @@ private:
     const SfxItemSet&           operator=(const SfxItemSet &) = delete;
 
 protected:
-    void                        PutDirect(const SfxPoolItem &rItem);
-
-    virtual const SfxPoolItem*  PutImpl( const SfxPoolItem&, sal_uInt16 nWhich, bool bPassingOwnership );
+    virtual const SfxPoolItem*  PutImpl( const SfxPoolItem&, sal_uInt16 nWhich, bool bItemIsSetMember, bool bPassingOwnership );
 
     /** special constructor for SfxAllItemSet */
     enum class SfxAllItemSetFlag { Flag };
@@ -166,15 +164,17 @@ public:
 
     sal_uInt16                  GetWhichByOffset(sal_uInt16 nOffset) const;
 
-    SfxItemState                GetItemState(   sal_uInt16 nWhich,
-                                                bool bSrchInParent = true,
-                                                const SfxPoolItem **ppItem = nullptr ) const;
+    SfxItemState GetItemState(sal_uInt16 nWhich, bool bSrchInParent = true, const SfxPoolItem **ppItem = nullptr) const
+    {
+        // use local helper, start value for looped-through SfxItemState value is SfxItemState::UNKNOWN
+        return GetItemState_ForWhichID(SfxItemState::UNKNOWN, nWhich, bSrchInParent, ppItem);
+    }
 
-    template <class T>
-    SfxItemState                GetItemState(   TypedWhichId<T> nWhich,
-                                                bool bSrchInParent = true,
-                                                const T **ppItem = nullptr ) const
-    { return GetItemState(sal_uInt16(nWhich), bSrchInParent, reinterpret_cast<SfxPoolItem const**>(ppItem)); }
+    template <class T> SfxItemState GetItemState(TypedWhichId<T> nWhich, bool bSrchInParent = true, const T **ppItem = nullptr ) const
+    {
+        // use local helper, start value for looped-through SfxItemState value is SfxItemState::UNKNOWN
+        return GetItemState_ForWhichID(SfxItemState::UNKNOWN, sal_uInt16(nWhich), bSrchInParent, reinterpret_cast<SfxPoolItem const**>(ppItem));
+    }
 
     /// Templatized version of GetItemState() to directly return the correct type.
     template<class T>
@@ -182,7 +182,7 @@ public:
                                                 bool bSrchInParent = true ) const
     {
         const SfxPoolItem * pItem = nullptr;
-        if( SfxItemState::SET == GetItemState(sal_uInt16(nWhich), bSrchInParent, &pItem) )
+        if (SfxItemState::SET == GetItemState_ForWhichID(SfxItemState::UNKNOWN, sal_uInt16(nWhich), bSrchInParent, &pItem))
             return static_cast<const T*>(pItem);
         return nullptr;
     }
@@ -193,7 +193,8 @@ public:
     { return HasItem(sal_uInt16(nWhich), reinterpret_cast<const SfxPoolItem**>(ppItem)); }
 
     void                        DisableItem(sal_uInt16 nWhich);
-    void                        InvalidateItem( sal_uInt16 nWhich );
+    void InvalidateItem(sal_uInt16 nWhich)
+    { InvalidateItem_ForWhichID(nWhich); }
     sal_uInt16                  ClearItem( sal_uInt16 nWhich = 0);
     void                        ClearInvalidItems();
     void                        InvalidateAllItems(); // HACK(via nWhich = 0) ???
@@ -203,9 +204,9 @@ public:
     // add, delete items, work on items
 public:
     const SfxPoolItem*          Put( const SfxPoolItem& rItem, sal_uInt16 nWhich )
-    { return PutImpl(rItem, nWhich, /*bPassingOwnership*/false); }
+    { return PutImpl(rItem, nWhich, /*bItemIsSetMember*/false, /*bPassingOwnership*/false); }
     const SfxPoolItem*          Put( std::unique_ptr<SfxPoolItem> xItem, sal_uInt16 nWhich )
-    { return PutImpl(*xItem.release(), nWhich, /*bPassingOwnership*/true); }
+    { return PutImpl(*xItem.release(), nWhich, /*bItemIsSetMember*/false, /*bPassingOwnership*/true); }
     const SfxPoolItem*          Put( const SfxPoolItem& rItem )
                                 { return Put(rItem, rItem.Which()); }
     const SfxPoolItem*          Put( std::unique_ptr<SfxPoolItem> xItem )
@@ -252,13 +253,22 @@ private:
     sal_uInt16 ClearSingleItem_ForWhichID( sal_uInt16 nWhich );
     sal_uInt16 ClearSingleItem_ForOffset( sal_uInt16 nOffset );
 
+    // cleanup all Items, but do not reset/change m_ppItems array. That is
+    // responsibility of the caller & allows specific resets
     sal_uInt16 ClearAllItemsImpl();
 
+    // Merge two given Item(entries)
+    void MergeItem_Impl(const SfxPoolItem **ppFnd1, const SfxPoolItem *pFnd2, bool bItemIsSetMember, bool bIgnoreDefaults);
+
+    // split version(s) of InvalidateItem for input types WhichID and Offset
+    void InvalidateItem_ForWhichID(sal_uInt16 nWhich);
+    void InvalidateItem_ForOffset(sal_uInt16 nOffset);
+
     // split version(s) of GetItemStateImpl for input types WhichID and Offset
     SfxItemState GetItemState_ForWhichID( SfxItemState eState, sal_uInt16 nWhich, bool bSrchInParent, const SfxPoolItem **ppItem) const;
     SfxItemState GetItemState_ForOffset( sal_uInt16 nOffset, const SfxPoolItem **ppItem) const;
 
-    void implCreateItemEntry(SfxPoolItem const*& rpTarget, SfxPoolItem const* pSource);
+    SfxPoolItem const* implCreateItemEntry(SfxPoolItem const* pSource, sal_uInt16 nWhich, bool bItemIsSetMember, bool bPassingOwnership);
     void implCleanupItemEntry(SfxPoolItem const* pSource);
 };
 
@@ -279,7 +289,7 @@ public:
 
     virtual std::unique_ptr<SfxItemSet> Clone( bool bItems = true, SfxItemPool *pToPool = nullptr ) const override;
 private:
-    virtual const SfxPoolItem*  PutImpl( const SfxPoolItem&, sal_uInt16 nWhich, bool bPassingOwnership ) override;
+    virtual const SfxPoolItem*  PutImpl( const SfxPoolItem&, sal_uInt16 nWhich, bool bItemIsSetMember,bool bPassingOwnership ) override;
 };