Cleanup some ScPatternAttr specific stuff

Removed some special stuff in the ItemSet/Item tooling regarding
ScPatternAttr. There are still quite a view (grep for
isExceptionalSCItem), but all these are identified and isolated.

Only for that Item (of over 500) are these exceptions, so that raelly
does not fit into the Item schemata in any way. Not even the pool
default of ScPatternAttr is without trouble. It is the only and last
Item that needs to be 'pooled' in the sense to avoid copies on any
price. That is OK, but should not be done in the ItemSet schemata.

In short: It should not be an Item at all. It should be changed
and renamed to something describing it's role (CellAttributes..?)
and get helped/assisted by something called CellAttributeHelper at
SC's model or Pool. It should not even be derived from SetItem, it
could just contain an shared_ptr of SfxItemSet (allows more and better
optimizations - think about Clone() and op==).

In parallel, all these hacks in the ItemSet/Item stuff could be
removed, making that faster and easier. Also quite some usages of
DirectPutItemInPool could be cleaned-up, this only exists since
there *is* no defined place to hold that data (coud be
CellAttributeHelper in the future). Putting Items directly to the
pool (and in most cases never remove them again) is just nonsense
and another hint that all this does not fit to the Item/ItemSet
schema at all.

This is now - after hard isolation of problems and have all working -
doable. It may be one of the next things to do, but there are
other candidates, too. Doing this would mostly only help SC...

Found another hack that uses DirectPutItemInPool and *never* removes
any Item again, see comments framelinkarray.cxx

And another one: PoolItemTest::testPool() explicitely tests
DirectPutItemInPool stuff - which makes no sense long-term,
but for now keep it and make it work by marking the slots used
as _bNeedsPoolRegistration == true

Have now overhauled the framelinkarray stuff to work without
DirectPutItemInPool and Cell not being a SfxPoolItem. That will
be much less complex and use much less calls/checks. Since this
is the data structure created for every calc repaint that should
get faster, too.

Also for now and memory loss security I added code to
DirectPutItemInPool to behave the same as with the unchanged
implCreateItemEntry: register Items so that the garbage collection
still is used. This will/can be removed when all usages of
DirectPutItemInPool is cleaned up.

Directly registering in DirectPutItemInPoolImpl is more tricky
than thought, but a good thing to do. Looking at that I saw that
tryRegisterSfxPoolItem is not used anymore, so rearranged some stuff.

Change-Id: If07762b0a25e2739027f81872441772dc82a25d9
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/159685
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <[email protected]>

include/svl/itempool.hxx

@@ -74,7 +74,7 @@ class SVL_DLLPUBLIC SfxItemPool : public salhelper::SimpleReferenceObject
     friend class SfxAllItemSet;
 
     // allow ItemSetTooling to access
-    friend SfxPoolItem const* implCreateItemEntry(SfxItemPool&, SfxPoolItem const*, sal_uInt16, bool, bool);
+    friend SfxPoolItem const* implCreateItemEntry(SfxItemPool&, SfxPoolItem const*, sal_uInt16, bool);
     friend void implCleanupItemEntry(SfxItemPool&, SfxPoolItem const*);
 
     // unit testing
@@ -230,16 +230,8 @@ public:
     static bool                     IsSlot(sal_uInt16 nId) {
                                         return nId && nId > SFX_WHICH_MAX; }
 
-    // this method tries to register an Item at this Pool. If this
-    // is done depends on the SfxItemInfo-flag _bNeedsPoolRegistration
-    // which needs to be set for Items that are accessed using
-    // GetItemSurrogates, else the Item will not be returned/accessed
-    void tryRegisterSfxPoolItem(const SfxPoolItem& rItem, bool bPoolDirect);
-
-    // this method will register the Item at this Pool, no matter what.
-    // It is needed for all calls that directly register Items at the
-    // Pool, so the DirectPutItemInPool-methods
-    void doRegisterSfxPoolItem(const SfxPoolItem& rItem);
+    // This method will try to register the Item at this Pool.
+    void registerSfxPoolItem(const SfxPoolItem& rItem);
 
     // this method will unregister an Item from this Pool
     void unregisterSfxPoolItem(const SfxPoolItem& rItem);

include/svl/itemset.hxx

@@ -37,7 +37,7 @@ SVL_DLLPUBLIC size_t getUsedSfxItemSetCount();
 #endif
 
 // ItemSet/ItemPool helpers
-SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource, sal_uInt16 nWhich, bool bPassingOwnership, bool bPoolDirect);
+SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource, sal_uInt16 nWhich, bool bPassingOwnership);
 void implCleanupItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource);
 
 class SAL_WARN_UNUSED SVL_DLLPUBLIC SfxItemSet
@@ -46,7 +46,7 @@ class SAL_WARN_UNUSED SVL_DLLPUBLIC SfxItemSet
     friend class SfxWhichIter;
 
     // allow ItemSetTooling to access
-    friend SfxPoolItem const* implCreateItemEntry(SfxItemPool&, SfxPoolItem const*, sal_uInt16, bool, bool);
+    friend SfxPoolItem const* implCreateItemEntry(SfxItemPool&, SfxPoolItem const*, sal_uInt16, bool);
     friend void implCleanupItemEntry(SfxItemPool&, SfxPoolItem const*);
 
     SfxItemPool*      m_pPool;         ///< pool that stores the items

include/svl/poolitem.hxx

@@ -115,7 +115,7 @@ class SVL_DLLPUBLIC SfxPoolItem
     friend class SfxItemSet;
 
     // allow ItemSetTooling to access
-    friend SfxPoolItem const* implCreateItemEntry(SfxItemPool&, SfxPoolItem const*, sal_uInt16, bool, bool);
+    friend SfxPoolItem const* implCreateItemEntry(SfxItemPool&, SfxPoolItem const*, sal_uInt16, bool);
     friend void implCleanupItemEntry(SfxItemPool&, SfxPoolItem const*);
 
     mutable sal_uInt32 m_nRefCount;
@@ -136,7 +136,7 @@ class SVL_DLLPUBLIC SfxPoolItem
     bool        m_bStaticDefault : 1;       // bit 2
     bool        m_bPoolDefault : 1;         // bit 3
     bool        m_bRegisteredAtPool : 1;    // bit 4
-    bool        m_bNewItemCallback : 1;     // bit 5
+    bool        m_bExceptionalSCItem : 1;     // bit 5
     bool        m_bIsSetItem : 1;           // bit 6
 
 protected:
@@ -158,7 +158,7 @@ protected:
     void setStaticDefault() { m_bStaticDefault = true; }
     void setPoolDefault() { m_bPoolDefault = true; }
     void setRegisteredAtPool(bool bNew) { m_bRegisteredAtPool = bNew; }
-    void setNewItemCallback() { m_bNewItemCallback = true; }
+    void setExceptionalSCItem() { m_bExceptionalSCItem = true; }
     void setIsSetItem() { m_bIsSetItem = true; }
 
 public:
@@ -178,7 +178,7 @@ public:
     bool isStaticDefault() const { return m_bStaticDefault; }
     bool isPoolDefault() const { return m_bPoolDefault; }
     bool isRegisteredAtPool() const { return m_bRegisteredAtPool; }
-    bool isNewItemCallback() const { return m_bNewItemCallback; }
+    bool isExceptionalSCItem() const { return m_bExceptionalSCItem; }
     bool isSetItem() const { return m_bIsSetItem; }
 
     // version that allows nullptrs

sc/source/core/data/patattr.cxx

@@ -76,7 +76,7 @@ ScPatternAttr::ScPatternAttr( SfxItemSet&& pItemSet, const OUString& rStyleName
         pStyle      ( nullptr ),
         mnPAKey(0)
 {
-    setNewItemCallback();
+    setExceptionalSCItem();
 
     // We need to ensure that ScPatternAttr is using the correct WhichRange,
     // see comments in commit message. This does transfers the items with
@@ -90,7 +90,7 @@ ScPatternAttr::ScPatternAttr( SfxItemSet&& pItemSet )
         pStyle      ( nullptr ),
         mnPAKey(0)
 {
-    setNewItemCallback();
+    setExceptionalSCItem();
 
     // We need to ensure that ScPatternAttr is using the correct WhichRange,
     // see comments in commit message. This does transfers the items with
@@ -104,7 +104,7 @@ ScPatternAttr::ScPatternAttr( SfxItemPool* pItemPool )
         pStyle      ( nullptr ),
         mnPAKey(0)
 {
-    setNewItemCallback();
+    setExceptionalSCItem();
 }
 
 ScPatternAttr::ScPatternAttr( const ScPatternAttr& rPatternAttr )
@@ -113,7 +113,7 @@ ScPatternAttr::ScPatternAttr( const ScPatternAttr& rPatternAttr )
         pStyle      ( rPatternAttr.pStyle ),
         mnPAKey(rPatternAttr.mnPAKey)
 {
-    setNewItemCallback();
+    setExceptionalSCItem();
 }
 
 ScPatternAttr* ScPatternAttr::Clone( SfxItemPool *pPool ) const

svl/qa/unit/items/test_itempool.cxx

@@ -37,7 +37,7 @@ void PoolItemTest::testPool()
     SfxItemInfo const aItems[] =
     {
         // _nSID, _bNeedsPoolRegistration, _bShareable
-        { 4, false, true },
+        { 4, true,  true },
         { 3, true,  false /* test NeedsPoolRegistration */ },
         { 2, false, false },
         { 1, true,  false /* test NeedsPoolRegistration */}

svl/source/items/itempool.cxx

@@ -801,8 +801,19 @@ const SfxPoolItem& SfxItemPool::DirectPutItemInPoolImpl(const SfxPoolItem& rItem
     nRemainingDirectlyPooledSfxPoolItemCount++;
 #endif
 
+    // CAUTION: Do not register the problematic pool default
+    if (rItem.isExceptionalSCItem() && GetMasterPool()->newItem_UseDirect(rItem))
+        return rItem;
+
     // make sure to use 'master'-pool, that's the one used by SfxItemSets
-    return *implCreateItemEntry(*GetMasterPool(), &rItem, nWhich, bPassingOwnership, true);
+    const SfxPoolItem* pRetval(implCreateItemEntry(*GetMasterPool(), &rItem, nWhich, bPassingOwnership));
+
+    // For the moment, as long as DirectPutItemInPoolImpl is used, make sure that
+    // the changes in implCreateItemEntry do not change anything, that would
+    // risc memory leaks by not (ab)using the garbage collector aspect of the pool.
+    registerSfxPoolItem(*pRetval);
+
+    return *pRetval;
 }
 
 void SfxItemPool::DirectRemoveItemFromPool(const SfxPoolItem& rItem)
@@ -1058,7 +1069,7 @@ sal_uInt16 SfxItemPool::GetTrueSlotId( sal_uInt16 nWhich ) const
     return pItemInfos[nWhich - pImpl->mnStart]._nSID;
 }
 
-void SfxItemPool::tryRegisterSfxPoolItem(const SfxPoolItem& rItem, bool bPoolDirect)
+void SfxItemPool::registerSfxPoolItem(const SfxPoolItem& rItem)
 {
     assert(rItem.Which() != 0);
 
@@ -1067,57 +1078,21 @@ void SfxItemPool::tryRegisterSfxPoolItem(const SfxPoolItem& rItem, bool bPoolDir
         return;
 
     if (rItem.isRegisteredAtPool())
-        // already registered, done (should not happen)
+        // already registered, done
         return;
 
     if (!IsInRange(rItem.Which()))
     {
         // get to the right pool
         if (pImpl->mpSecondary)
         {
-            pImpl->mpSecondary->tryRegisterSfxPoolItem(rItem, bPoolDirect);
+            pImpl->mpSecondary->registerSfxPoolItem(rItem);
             return;
         }
 
         return;
     }
 
-    // get index (must exist due to checks above)
-    const sal_uInt16 nIndex(rItem.Which() - pImpl->mnStart);
-    bool bRegisterItem(bPoolDirect);
-
-    if (!bRegisterItem)
-    {
-        if (g_bItemClassicMode)
-        {
-            // classic mode: register in general *all* items
-            // ...but only shareable ones: for non-shareable registering for re-use
-            // makes no sense
-            bRegisterItem = Shareable_Impl(nIndex);
-        }
-        else
-        {
-            // experimental mode: only register items that are defined to be registered
-            bRegisterItem = NeedsPoolRegistration_Impl(nIndex);
-        }
-    }
-
-    if (bRegisterItem)
-        doRegisterSfxPoolItem(rItem);
-}
-
-void SfxItemPool::doRegisterSfxPoolItem(const SfxPoolItem& rItem)
-{
-    assert(rItem.Which() != 0);
-
-    if (IsSlot(rItem.Which()))
-        // do not register SlotItems
-        return;
-
-    if (rItem.isRegisteredAtPool())
-        // already registered, done
-        return;
-
     if (nullptr == ppRegisteredSfxPoolItems)
         // on-demand allocate array of registeredSfxPoolItems and init to nullptr
         ppRegisteredSfxPoolItems = new registeredSfxPoolItems*[GetSize_Impl()]{};

svl/source/items/itemset.cxx

@@ -128,7 +128,7 @@ SfxItemSet::SfxItemSet(SfxItemPool& pool, WhichRangesContainer wids)
     assert(svl::detail::validRanges2(m_pWhichRanges));
 }
 
-SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource, sal_uInt16 nWhich, bool bPassingOwnership, bool bPoolDirect)
+SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource, sal_uInt16 nWhich, bool bPassingOwnership)
 {
     if (nullptr == pSource)
         // SfxItemState::UNKNOWN aka current default (nullptr)
@@ -140,7 +140,7 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
         // just use pSource which equals INVALID_POOL_ITEM
         return pSource;
 
-    if (pSource->isNewItemCallback() && rPool.GetMasterPool()->newItem_UseDirect(*pSource))
+    if (pSource->isExceptionalSCItem() && rPool.GetMasterPool()->newItem_UseDirect(*pSource))
         // exceptional handling for *some* items, see SC
         // (do not copy item: use directly, it is a pool default)
         return pSource;
@@ -240,22 +240,14 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
         return pSource;
     }
 
-    // classic mode: try finding already existing item
-    // NOTE: bPoolDirect currently required due to DirectPutItemInPool and the
-    //   self-handled Item ScPatternAttr/ATTR_PATTERN in SC, else e.g.
-    //   testIteratorsDefPattern will fail in line 1306
+    // g_bItemClassicMode: try finding already existing item
     // NOTE: the UnitTest testIteratorsDefPattern claims that that Item "can be
     //   edited by the user" which explains why it breaks so many rules for Items,
     //   it behaves like an alien. That Item in the SC Pool claims to be a
     //   'StaticDefault' and gets changed (..?)
-    // NOTE: despite 1st thinking that this can be limited to ScPatternAttr/
-    //   ATTR_PATTERN it also has to be applied to the range
-    //   [ATTR_PATTERN_START, ATTR_PATTERN_END] *used* by ATTR_PATTERN, plus
-    //   it, so it's [100 .. 155] and [156] in SC. For now, just use bPoolDirect.
-    //   This needs to be cleaned-up somehow anyways
 
     // only do this if classic mode or required (calls from Pool::Direct*)
-    while(g_bItemClassicMode || bPoolDirect)
+    while(g_bItemClassicMode || pSource->isExceptionalSCItem())
     {
         if (!pTargetPool->Shareable_Impl(nIndex))
             // not shareable, so no need to search for identical item
@@ -308,29 +300,31 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
 
     // Unfortunately e,g, SC does 'special' things for some new Items,
     // so we need to give the opportunity for this. To limit this to
-    // the needed cases, use m_bNewItemCallback flag at item
-    if (pSource->isNewItemCallback())
+    // the needed cases, use m_bExceptionalSCItem flag at item
+    if (pSource->isExceptionalSCItem())
         pMasterPool->newItem_Callback(*pSource);
 
     // try to register @Pool (only needed if not yet registered)
     if (!pSource->isRegisteredAtPool())
     {
-        if (!bPoolDirect) // re-use bPoolDirect
+        bool bRegisterAtPool(pSource->isExceptionalSCItem());
+
+        if (!bRegisterAtPool)
         {
             if (g_bItemClassicMode)
             {
                 // in classic mode register only/all shareable items
-                bPoolDirect = pTargetPool->Shareable_Impl(nIndex);
+                bRegisterAtPool = pTargetPool->Shareable_Impl(nIndex);
             }
             else
             {
                 // in new mode register only/all items marked as need to be registered
-                bPoolDirect = pTargetPool->NeedsPoolRegistration_Impl(nIndex);
+                bRegisterAtPool = pTargetPool->NeedsPoolRegistration_Impl(nIndex);
             }
         }
 
-        if (bPoolDirect)
-            pTargetPool->doRegisterSfxPoolItem(*pSource);
+        if (bRegisterAtPool)
+            pTargetPool->registerSfxPoolItem(*pSource);
     }
 
     return pSource;
@@ -346,7 +340,7 @@ void implCleanupItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource)
         // nothing to do for invalid item entries
         return;
 
-    if (pSource->isNewItemCallback() && rPool.GetMasterPool()->newItem_UseDirect(*pSource))
+    if (pSource->isExceptionalSCItem() && rPool.GetMasterPool()->newItem_UseDirect(*pSource))
         // exceptional handling for *some* items, see SC
         // do not delete Item, it is a pool default
         return;
@@ -421,7 +415,7 @@ SfxItemSet::SfxItemSet( const SfxItemSet& rASet )
 
     for (const auto& rSource : rASet)
     {
-        *ppDst = implCreateItemEntry(*GetPool(), rSource, 0, false, false);
+        *ppDst = implCreateItemEntry(*GetPool(), rSource, 0, false);
         ppDst++;
     }
 
@@ -702,7 +696,7 @@ const SfxPoolItem* SfxItemSet::PutImpl(const SfxPoolItem& rItem, sal_uInt16 nWhi
     }
 
     // prepare new entry
-    SfxPoolItem const* pNew(implCreateItemEntry(*GetPool(), &rItem, nWhich, bPassingOwnership, false));
+    SfxPoolItem const* pNew(implCreateItemEntry(*GetPool(), &rItem, nWhich, bPassingOwnership));
 
     // Notification-Callback
     if(m_aCallback)
@@ -1311,7 +1305,7 @@ void SfxItemSet::MergeItem_Impl(const SfxPoolItem **ppFnd1, const SfxPoolItem *p
 
         else if ( pFnd2 && bIgnoreDefaults )
             // Decision table: default, set, doesn't matter, sal_True
-            *ppFnd1 = implCreateItemEntry(*GetPool(), pFnd2, 0, false, false);
+            *ppFnd1 = implCreateItemEntry(*GetPool(), pFnd2, 0, false);
             // *ppFnd1 = &GetPool()->Put( *pFnd2 );
 
         if ( *ppFnd1 )

svl/source/items/poolitem.cxx

@@ -499,7 +499,7 @@ SfxPoolItem::SfxPoolItem(sal_uInt16 const nWhich)
     , m_bStaticDefault(false)
     , m_bPoolDefault(false)
     , m_bRegisteredAtPool(false)
-    , m_bNewItemCallback(false)
+    , m_bExceptionalSCItem(false)
     , m_bIsSetItem(false)
 #ifdef DBG_UTIL
     , m_bDeleted(false)

svx/source/dialog/framelink.cxx

@@ -178,6 +178,10 @@ Style& Style::MirrorSelf()
 
 bool Style::operator==( const Style& rOther) const
 {
+    if (this == &rOther)
+        // ptr compare (same instance)
+        return true;
+
     return (Prim() == rOther.Prim()
         && Dist() == rOther.Dist()
         && Secn() == rOther.Secn()