ITEM: preparations for more/easier changes

This change is not about speed improvements but diverse
preparations to make changes/reading/understanding easier.
It does not change speed AFAIK.

Added a global static debug-only counter to allow getting
an overview over number of all allocated SfxPoolItem's
and the still alloated ones at office shutdown. The values
are used in Application::~Application to make a short info
statement. It allows to be able to quickly detect if an
error in future changes may lead to memory losses - these
would show in dramaitically higher numbers then (hopefully)
immediately.

Moved SfxVoidItem to own source/header.

Added container library interface support to SfxItemSet,
adapted already some methods to use it - not all possible,
I will commit & get status from gerrit 1st if all still works
and then continue.

Changed INVALID_POOL_ITEM from -1 to use a global unique
incarnation of an isolated derivation from SfxPoolItem. It
allows to avoid the (-1) pointer hack. Since still just
pointers are compared it's not worse. NOTE: That way, more
'special' SfxPoolItem's may be used for more States - a
candidate is e.g. SfxVoidItem(0) which represents ::DISABLED
state -- unfortunately not only, it is also used (mainly for
UI stuff) with 'real' WhichIDs - hard to sort out, will have
to stay that way for now AFAIK.

Changed INVALID_POOL_ITEM stuff to use a static extern
incarnated item in combination with a inline method
to return it, called GetGlobalStaticInvalidItemInstance().

Isolated create/cleanup of a SfxPoolItem entry in
SfxItemSet to further modularize/simplify that. It is
currently from constructor & destructor but already shows
that PoolDefaults are handled differently - probably an
error. Still, for now, do no change in behaviour (yet).

Got regular 'killed by the Kill-Wrapper' messages from
gerrit, seems to have to do with UITest_sw_findReplace.
That python/c++ scripting stuff is hard to debug, but
finally I identified the problem has to do with
the INVALID_POOL_ITEM change. It was in
SfxItemSet::InvalidateAllItems() where still a (-1)
was used -> chaos in detecting invalid items.

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

5 files changed, 170 insertions, 75 deletions

diff --git a/svl/Library_svl.mk b/svl/Library_svl.mk
index 87024bbf499e..aee540d56514 100644
--- a/svl/Library_svl.mk
+++ b/svl/Library_svl.mk
@@ -143,6 +143,7 @@ $(eval $(call gb_Library_add_exception_objects,svl,\
     svl/source/items/style \
     svl/source/items/stylepool \
     svl/source/items/visitem \
+    svl/source/items/voiditem \
     svl/source/items/whiter \
     svl/source/misc/PasswordHelper \
     svl/source/misc/adrparse \
diff --git a/svl/qa/unit/items/test_itempool.cxx b/svl/qa/unit/items/test_itempool.cxx
index 6868999f0f76..339da002bfc1 100644
--- a/svl/qa/unit/items/test_itempool.cxx
+++ b/svl/qa/unit/items/test_itempool.cxx
@@ -8,6 +8,7 @@
  */
 
 #include <svl/itempool.hxx>
+#include <svl/voiditem.hxx>
 #include <poolio.hxx>
 
 #include <cppunit/TestAssert.h>
diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx
index 289a69adf4da..401044e8ba09 100644
--- a/svl/source/items/itemset.cxx
+++ b/svl/source/items/itemset.cxx
@@ -32,6 +32,7 @@
 #include <svl/itemiter.hxx>
 #include <svl/setitem.hxx>
 #include <svl/whiter.hxx>
+#include <svl/voiditem.hxx>
 
 #include <items_helper.hxx>
 
@@ -99,6 +100,68 @@ SfxItemSet::SfxItemSet(SfxItemPool& pool, WhichRangesContainer wids)
     assert(svl::detail::validRanges2(m_pWhichRanges));
 }
 
+void SfxItemSet::implCreateItemEntry(SfxPoolItem const*& rpTarget, SfxPoolItem const* pSource)
+{
+    if (nullptr == pSource
+        || IsInvalidItem(pSource)
+        || IsStaticDefaultItem(pSource))
+    {
+        // copy the pointer if
+        // - SfxItemState::UNKNOWN aka current default (nullptr)
+        // - SfxItemState::DONTCARE aka invalid item
+        // - StaticDefaultItem aka static pool default (however this finds
+        //   it's way to the ItemSet? It *should* be default, too)
+        rpTarget = pSource;
+        return;
+    }
+
+    if (0 == pSource->Which())
+    {
+        // these *should* be SfxVoidItem(0) the only Items with 0 == WhichID
+        rpTarget = pSource->Clone();
+        return;
+    }
+
+    if (m_pPool->IsItemPoolable(*pSource))
+    {
+        // copy the pointer and increase RefCount
+        rpTarget = pSource;
+        rpTarget->AddRef();
+        return;
+    }
+
+    // !IsPoolable() => assign via Pool
+    rpTarget = &m_pPool->Put(*pSource);
+}
+
+void SfxItemSet::implCleanupItemEntry(SfxPoolItem const* pSource)
+{
+    if (nullptr == pSource         // no entry, done
+        || IsInvalidItem(pSource)  // nothing to do for invalid item entries
+        || IsDefaultItem(pSource)) // default items are owned by the pool, nothing to do
+    {
+        return;
+    }
+
+    if (0 == pSource->Which())
+    {
+        // these *should* be SfxVoidItem(0) the only Items with 0 == WhichID
+        // and need to be deleted
+        delete pSource;
+        return;
+    }
+
+    if (1 < pSource->GetRefCount())
+    {
+        // Still multiple references present, so just alter the RefCount
+        pSource->ReleaseRef();
+        return;
+    }
+
+    // Delete from Pool
+    m_pPool->Remove(*pSource);
+}
+
 SfxItemSet::SfxItemSet( const SfxItemSet& rASet )
     : m_pPool( rASet.m_pPool )
     , m_pParent( rASet.m_pParent )
@@ -114,28 +177,17 @@ SfxItemSet::SfxItemSet( const SfxItemSet& rASet )
         return;
     }
 
-    m_ppItems = new const SfxPoolItem* [TotalCount()] {};
+    // allocate new array (no need to initialize, will be done below)
+    m_ppItems = new const SfxPoolItem* [TotalCount()];
 
     // Copy attributes
-    SfxPoolItem const** ppDst = m_ppItems;
-    SfxPoolItem const** ppSrc = rASet.m_ppItems;
-    for( sal_uInt16 n = TotalCount(); n; --n, ++ppDst, ++ppSrc )
-        if ( nullptr == *ppSrc ||                 // Current Default?
-             IsInvalidItem(*ppSrc) ||       // DontCare?
-             IsStaticDefaultItem(*ppSrc) )  // Defaults that are not to be pooled?
-            // Just copy the pointer
-            *ppDst = *ppSrc;
-        else if (m_pPool->IsItemPoolable( **ppSrc ))
-        {
-            // Just copy the pointer and increase RefCount
-            *ppDst = *ppSrc;
-            (*ppDst)->AddRef();
-        }
-        else if ( !(*ppSrc)->Which() )
-            *ppDst = (*ppSrc)->Clone();
-        else
-            // !IsPoolable() => assign via Pool
-            *ppDst = &m_pPool->Put( **ppSrc );
+    SfxPoolItem const** ppDst(m_ppItems);
+
+    for (const auto& rSource : rASet)
+    {
+        implCreateItemEntry(*ppDst, rSource);
+        ppDst++;
+    }
 
     assert(svl::detail::validRanges2(m_pWhichRanges));
 }
@@ -170,32 +222,24 @@ SfxItemSet::SfxItemSet(SfxItemSet&& rASet) noexcept
 
 SfxItemSet::~SfxItemSet()
 {
-    if (!m_pWhichRanges.empty()) // might be nullptr if we have been moved-from
+    // might be nullptr if we have been moved-from, also only needed
+    // when we have ::SET items
+    if (!m_pWhichRanges.empty() && Count())
     {
-        if( Count() )
+        for (auto& rCandidate : *this)
         {
-            SfxPoolItem const** ppFnd = m_ppItems;
-            for( sal_uInt16 nCnt = TotalCount(); nCnt; --nCnt, ++ppFnd )
-                if( *ppFnd && !IsInvalidItem(*ppFnd) )
-                {
-                    if( !(*ppFnd)->Which() )
-                        delete *ppFnd;
-                    else {
-                        // Still multiple references present, so just alter the RefCount
-                        if ( 1 < (*ppFnd)->GetRefCount() && !IsDefaultItem(*ppFnd) )
-                            (*ppFnd)->ReleaseRef();
-                        else
-                            if ( !IsDefaultItem(*ppFnd) )
-                                // Delete from Pool
-                                m_pPool->Remove( **ppFnd );
-                    }
-                }
+            implCleanupItemEntry(rCandidate);
         }
     }
 
     if (!m_bItemsFixed)
+    {
+        // free SfxPoolItem array
         delete[] m_ppItems;
-    m_pWhichRanges.reset(); // for invariant-testing
+    }
+
+    // for invariant-testing
+    m_pWhichRanges.reset();
 }
 
 /**
@@ -316,7 +360,7 @@ void SfxItemSet::ClearInvalidItems()
 void SfxItemSet::InvalidateAllItems()
 {
     assert( !m_nCount && "There are still Items set" );
-    memset(static_cast<void*>(m_ppItems), -1, TotalCount() * sizeof(SfxPoolItem*));
+    std::fill(m_ppItems, m_ppItems + TotalCount(), INVALID_POOL_ITEM);
 }
 
 SfxItemState SfxItemSet::GetItemState( sal_uInt16 nWhich,
diff --git a/svl/source/items/poolitem.cxx b/svl/source/items/poolitem.cxx
index cb1bec15159c..9a45e8803448 100644
--- a/svl/source/items/poolitem.cxx
+++ b/svl/source/items/poolitem.cxx
@@ -461,16 +461,30 @@
 // class SwPaMItem : public SfxPoolItem
 //////////////////////////////////////////////////////////////////////////////
 
+#ifdef DBG_UTIL
+static size_t nAllocatedSfxPoolItemCount(0);
+static size_t nUsedSfxPoolItemCount(0);
+size_t getAllocatedSfxPoolItemCount() { return nAllocatedSfxPoolItemCount; }
+size_t getUsedSfxPoolItemCount() { return nUsedSfxPoolItemCount; }
+#endif
+
 SfxPoolItem::SfxPoolItem(sal_uInt16 const nWhich)
     : m_nRefCount(0)
     , m_nWhich(nWhich)
     , m_nKind(SfxItemKind::NONE)
 {
+#ifdef DBG_UTIL
+    nAllocatedSfxPoolItemCount++;
+    nUsedSfxPoolItemCount++;
+#endif
     assert(nWhich <= SHRT_MAX);
 }
 
 SfxPoolItem::~SfxPoolItem()
 {
+#ifdef DBG_UTIL
+    nAllocatedSfxPoolItemCount--;
+#endif
     assert((m_nRefCount == 0 || m_nRefCount > SFX_ITEMS_MAXREF) && "destroying item in use");
 }
 
@@ -562,40 +576,6 @@ std::unique_ptr<SfxPoolItem> SfxPoolItem::CloneSetWhich(sal_uInt16 nNewWhich) co
 
 bool SfxPoolItem::IsVoidItem() const { return false; }
 
-SfxPoolItem* SfxVoidItem::CreateDefault() { return new SfxVoidItem(0); }
-
-SfxVoidItem::SfxVoidItem(sal_uInt16 which)
-    : SfxPoolItem(which)
-{
-}
-
-bool SfxVoidItem::operator==(const SfxPoolItem& rCmp) const
-{
-    assert(SfxPoolItem::operator==(rCmp));
-    (void)rCmp;
-    return true;
-}
-
-bool SfxVoidItem::GetPresentation(SfxItemPresentation /*ePresentation*/, MapUnit /*eCoreMetric*/,
-                                  MapUnit /*ePresentationMetric*/, OUString& rText,
-                                  const IntlWrapper&) const
-{
-    rText = "Void";
-    return true;
-}
-
-void SfxVoidItem::dumpAsXml(xmlTextWriterPtr pWriter) const
-{
-    (void)xmlTextWriterStartElement(pWriter, BAD_CAST("SfxVoidItem"));
-    (void)xmlTextWriterWriteAttribute(pWriter, BAD_CAST("whichId"),
-                                      BAD_CAST(OString::number(Which()).getStr()));
-    (void)xmlTextWriterEndElement(pWriter);
-}
-
-SfxVoidItem* SfxVoidItem::Clone(SfxItemPool*) const { return new SfxVoidItem(*this); }
-
-bool SfxVoidItem::IsVoidItem() const { return true; }
-
 void SfxPoolItem::ScaleMetrics(tools::Long /*lMult*/, tools::Long /*lDiv*/) {}
 
 bool SfxPoolItem::HasMetrics() const { return false; }
@@ -612,6 +592,16 @@ bool SfxPoolItem::PutValue(const css::uno::Any&, sal_uInt8)
     return false;
 }
 
-SfxVoidItem::~SfxVoidItem() {}
+namespace
+{
+class InvalidItem final : public SfxPoolItem
+{
+    virtual bool operator==(const SfxPoolItem&) const override { return true; }
+    virtual SfxPoolItem* Clone(SfxItemPool*) const override { return nullptr; }
+};
+InvalidItem aInvalidItem;
+}
+
+SfxPoolItem const* const INVALID_POOL_ITEM = &aInvalidItem;
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/svl/source/items/voiditem.cxx b/svl/source/items/voiditem.cxx
new file mode 100644
index 000000000000..3afa64de5332
--- /dev/null
+++ b/svl/source/items/voiditem.cxx
@@ -0,0 +1,59 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * This file incorporates work covered by the following license notice:
+ *
+ *   Licensed to the Apache Software Foundation (ASF) under one or more
+ *   contributor license agreements. See the NOTICE file distributed
+ *   with this work for additional information regarding copyright
+ *   ownership. The ASF licenses this file to you under the Apache
+ *   License, Version 2.0 (the "License"); you may not use this file
+ *   except in compliance with the License. You may obtain a copy of
+ *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
+ */
+
+#include <svl/voiditem.hxx>
+#include <libxml/xmlwriter.h>
+
+SfxPoolItem* SfxVoidItem::CreateDefault() { return new SfxVoidItem(0); }
+
+SfxVoidItem::SfxVoidItem(sal_uInt16 which)
+    : SfxPoolItem(which)
+{
+}
+
+bool SfxVoidItem::operator==(const SfxPoolItem& rCmp) const
+{
+    assert(SfxPoolItem::operator==(rCmp));
+    (void)rCmp;
+    return true;
+}
+
+bool SfxVoidItem::GetPresentation(SfxItemPresentation /*ePresentation*/, MapUnit /*eCoreMetric*/,
+                                  MapUnit /*ePresentationMetric*/, OUString& rText,
+                                  const IntlWrapper&) const
+{
+    rText = "Void";
+    return true;
+}
+
+void SfxVoidItem::dumpAsXml(xmlTextWriterPtr pWriter) const
+{
+    (void)xmlTextWriterStartElement(pWriter, BAD_CAST("SfxVoidItem"));
+    (void)xmlTextWriterWriteAttribute(pWriter, BAD_CAST("whichId"),
+                                      BAD_CAST(OString::number(Which()).getStr()));
+    (void)xmlTextWriterEndElement(pWriter);
+}
+
+SfxVoidItem* SfxVoidItem::Clone(SfxItemPool*) const { return new SfxVoidItem(*this); }
+
+bool SfxVoidItem::IsVoidItem() const { return true; }
+
+SfxVoidItem::~SfxVoidItem() {}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */