Revert "used std::map in SfxItemSet"

This reverts commit 2757ee9fe610e253e4ccc37423fa420004d0f388.

Besides causing a performance regression, I now notice that
there is code in SW that relies on iterating over two different
SfxItemSet's in parallel, and assumes that missing items are
returned as nullptr, which is not the case for my std::map based
change.

Change-Id: I2b1110350fe4c4b74e5508558e9661ef1e1a103e

include/svl/itemiter.hxx

@@ -21,7 +21,6 @@
 
 #include <svl/svldllapi.h>
 #include <svl/itemset.hxx>
-#include <vector>
 
 class SfxPoolItem;
 class SfxItemSet;
@@ -30,22 +29,31 @@ class SfxItemPool;
 class SVL_DLLPUBLIC SfxItemIter
 {
     const SfxItemSet&   m_rSet;
-    std::vector<sal_uInt16>      m_keys;
+    sal_uInt16 m_nStart;
-    std::vector<sal_uInt16>::const_iterator m_iter;
+    sal_uInt16 m_nEnd;
+    sal_uInt16 m_nCurrent;
 
 public:
     SfxItemIter( const SfxItemSet& rSet );
     ~SfxItemIter();
 
     /// get item, or null if no items
-    SfxPoolItem const * FirstItem();
+    const SfxPoolItem* FirstItem()
-    SfxPoolItem const * GetCurItem();
+    {
-    SfxPoolItem const * NextItem();
+        m_nCurrent = m_nStart;
+        return m_rSet.m_nCount ? *(m_rSet.m_pItems + m_nCurrent) : nullptr;
+    }
+    const SfxPoolItem* GetCurItem()
+    {
+        return m_rSet.m_nCount ? *(m_rSet.m_pItems + m_nCurrent) : nullptr;
+    }
+    const SfxPoolItem* NextItem();
 
-    bool       IsAtEnd()       const;
+    bool       IsAtEnd()     const { return m_nCurrent == m_nEnd; }
-    sal_uInt16 GetCurWhich()   const { return *m_iter; }
+
-    sal_uInt16 GetFirstWhich() const { return *m_keys.begin(); }
+    sal_uInt16 GetCurPos()   const { return m_nCurrent; }
-    sal_uInt16 GetLastWhich()  const { return *m_keys.rbegin(); }
+    sal_uInt16 GetFirstPos() const { return m_nStart; }
+    sal_uInt16 GetLastPos()  const { return m_nEnd; }
 };
 
 #endif

include/svl/itemset.hxx

@@ -23,13 +23,12 @@
 
 #include <cstdarg>
 #include <svl/poolitem.hxx>
-#include <map>
 
 class SfxItemPool;
 class SfxPoolItem;
 class SvStream;
 
-typedef std::map<sal_uInt16, SfxPoolItem const *> SfxItemMap;
+typedef SfxPoolItem const** SfxItemArray;
 
 class SAL_WARN_UNUSED SVL_DLLPUBLIC SfxItemSet
 {
@@ -37,8 +36,9 @@ class SAL_WARN_UNUSED SVL_DLLPUBLIC SfxItemSet
 
     SfxItemPool*      m_pPool;         ///< pool that stores the items
     const SfxItemSet* m_pParent;       ///< derivation
-    SfxItemMap        m_aItems;        ///< array of items
+    SfxItemArray      m_pItems;        ///< array of items
     sal_uInt16*       m_pWhichRanges;  ///< array of Which Ranges
+    sal_uInt16        m_nCount;        ///< number of items
 
 friend class SfxItemPoolCache;
 friend class SfxAllItemSet;
@@ -50,7 +50,7 @@ private:
     SVL_DLLPRIVATE void                     InitRanges_Impl(sal_uInt16 nWh1, sal_uInt16 nWh2);
 
 public:
-    SfxItemMap const &          GetItems_Impl() const { return m_aItems; }
+    SfxItemArray                GetItems_Impl() const { return m_pItems; }
 
 private:
     const SfxItemSet&           operator=(const SfxItemSet &) = delete;
@@ -73,7 +73,7 @@ public:
     virtual SfxItemSet *        Clone(bool bItems = true, SfxItemPool *pToPool = nullptr) const;
 
     // Get number of items
-    sal_uInt16                  Count() const { return m_aItems.size(); }
+    sal_uInt16                  Count() const { return m_nCount; }
     sal_uInt16                  TotalCount() const;
 
     const SfxPoolItem&          Get( sal_uInt16 nWhich, bool bSrchInParent = true ) const;

sc/source/core/data/patattr.cxx

@@ -126,10 +126,10 @@ inline bool EqualPatternSets( const SfxItemSet& rSet1, const SfxItemSet& rSet2 )
     if ( rSet1.Count() != rSet2.Count() )
         return false;
 
-    SfxItemMap const & rItems1 = rSet1.GetItems_Impl();   // inline method of SfxItemSet
+    SfxItemArray pItems1 = rSet1.GetItems_Impl();   // inline method of SfxItemSet
-    SfxItemMap const & rItems2 = rSet2.GetItems_Impl();
+    SfxItemArray pItems2 = rSet2.GetItems_Impl();
 
-    return rItems1 == rItems2;
+    return ( 0 == memcmp( pItems1, pItems2, (ATTR_PATTERN_END - ATTR_PATTERN_START + 1) * sizeof(pItems1[0]) ) );
 }
 
 bool ScPatternAttr::operator==( const SfxPoolItem& rCmp ) const

solenv/gdb/libreoffice/svl.py

@@ -34,8 +34,48 @@ class ItemSetPrinter(object):
         return whiches
 
     def children(self):
-        children = [ ( 'items', self.value['m_aItems'] ) ]
+        whichranges = self.which_ranges()
-        return children.__iter__()
+        size = 0
+        whichids = []
+        for (whichfrom, whichto) in whichranges:
+            size += whichto - whichfrom + 1
+            whichids += [which for which in range(whichfrom, whichto+1)]
+        return self._iterator(self.value['m_pItems'], size, whichids)
+
+    class _iterator(six.Iterator):
+
+        def __init__(self, data, count, whichids):
+            self.data = data
+            self.whichids = whichids
+            self.count = count
+            self.pos = 0
+            self._check_invariant()
+
+        def __iter__(self):
+            return self
+
+        def __next__(self):
+            if self.pos == self.count:
+                raise StopIteration()
+
+            which = self.whichids[self.pos]
+            elem = self.data[self.pos]
+            self.pos = self.pos + 1
+
+            self._check_invariant()
+            if (elem == -1):
+                elem = "(Invalid)"
+            elif (elem != 0):
+                # let's try how well that works...
+                elem = elem.cast(elem.dynamic_type).dereference()
+            return (str(which), elem)
+
+        def _check_invariant(self):
+            assert self.count >= 0
+            assert self.data
+            assert self.pos >= 0
+            assert self.pos <= self.count
+            assert len(self.whichids) == self.count
 
 printer = None
 

svl/qa/unit/items/test_itempool.cxx

@@ -8,8 +8,6 @@
  */
 
 #include <svl/itempool.hxx>
-#include <svl/itemset.hxx>
-#include <svl/itemiter.hxx>
 #include <poolio.hxx>
 
 #include <cppunit/TestAssert.h>
@@ -23,11 +21,13 @@ class PoolItemTest : public CppUnit::TestFixture
              PoolItemTest() {}
 
     void testPool();
-    void testItemSet();
 
+    // Adds code needed to register the test suite
     CPPUNIT_TEST_SUITE(PoolItemTest);
+
     CPPUNIT_TEST(testPool);
-    CPPUNIT_TEST(testItemSet);
+
+    // End of test suite definition
     CPPUNIT_TEST_SUITE_END();
 };
 
@@ -99,53 +99,6 @@ void PoolItemTest::testPool()
     CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), pImpl->maPoolItems[3]->maFree.size());
 }
 
-void PoolItemTest::testItemSet()
-{
-    SfxItemInfo aItems[] =
-        { { 1, false },
-          { 2, false },
-          { 3, false },
-          { 4, false },
-          { 5, false },
-          { 6, false },
-          { 7, false }
-        };
-
-    SfxItemPool *pPool = new SfxItemPool("testpool", 1, 7, aItems);
-    std::vector<SfxPoolItem*> aDefaults {
-        new SfxVoidItem(1),
-        new SfxVoidItem(2),
-        new SfxVoidItem(3),
-        new SfxVoidItem(4),
-        new SfxVoidItem(5),
-        new SfxVoidItem(6),
-        new SfxVoidItem(7)
-    };
-    pPool->SetDefaults(&aDefaults);
-
-    SfxItemSet aItemSet(*pPool, 1, 3, 5, 7, 0);
-    aItemSet.Put(SfxVoidItem(1));
-    aItemSet.Put(SfxVoidItem(2));
-    aItemSet.Put(SfxVoidItem(3));
-    aItemSet.Put(SfxVoidItem(5));
-    aItemSet.Put(SfxVoidItem(6));
-    aItemSet.Put(SfxVoidItem(7));
-
-    SfxItemIter aIter(aItemSet);
-
-    CPPUNIT_ASSERT_EQUAL((sal_uInt16)1, aIter.GetFirstWhich());
-    CPPUNIT_ASSERT_EQUAL((sal_uInt16)7, aIter.GetLastWhich());
-    const SfxPoolItem *pFirstItem = aIter.FirstItem();
-    CPPUNIT_ASSERT(pFirstItem);
-    CPPUNIT_ASSERT_EQUAL((sal_uInt16)1, pFirstItem->Which());
-    CPPUNIT_ASSERT_EQUAL((sal_uInt16)2, aIter.NextItem()->Which());
-    CPPUNIT_ASSERT_EQUAL((sal_uInt16)3, aIter.NextItem()->Which());
-    CPPUNIT_ASSERT_EQUAL((sal_uInt16)5, aIter.NextItem()->Which());
-    CPPUNIT_ASSERT_EQUAL((sal_uInt16)6, aIter.NextItem()->Which());
-    CPPUNIT_ASSERT_EQUAL((sal_uInt16)7, aIter.NextItem()->Which());
-    CPPUNIT_ASSERT_EQUAL(static_cast<const SfxPoolItem*>(nullptr), aIter.NextItem());
-    CPPUNIT_ASSERT_EQUAL(true, aIter.IsAtEnd());
-}
 
 CPPUNIT_TEST_SUITE_REGISTRATION(PoolItemTest);
 

svl/source/items/itemiter.cxx

@@ -25,50 +25,44 @@
 SfxItemIter::SfxItemIter( const SfxItemSet& rItemSet )
     : m_rSet( rItemSet )
 {
-    // store the set of keys because client code likes modifying the map
+    if (!m_rSet.m_nCount)
-    // while iterating over it
+    {
-    m_keys.resize(rItemSet.m_aItems.size());
+        m_nStart = 1;
-    size_t idx = 0;
+        m_nEnd = 0;
-    for (auto const & rPair : rItemSet.m_aItems) {
-        m_keys[idx++] = rPair.first;
     }
-    m_iter = m_keys.begin();
+    else
+    {
+        SfxItemArray ppFnd = m_rSet.m_pItems;
+
+        // Find the first Item that is set
+        for (m_nStart = 0; !*(ppFnd + m_nStart ); ++m_nStart)
+            ; // empty loop
+        if (1 < m_rSet.Count())
+            for (m_nEnd = m_rSet.TotalCount(); !*(ppFnd + --m_nEnd); )
+                ; // empty loop
+        else
+            m_nEnd = m_nStart;
+    }
+
+    m_nCurrent = m_nStart;
 }
 
 SfxItemIter::~SfxItemIter()
 {
 }
 
-SfxPoolItem const * SfxItemIter::FirstItem()
+const SfxPoolItem* SfxItemIter::NextItem()
 {
-    m_iter = m_keys.begin();
+    SfxItemArray ppFnd = m_rSet.m_pItems;
-    return GetCurItem();
-}
 
-SfxPoolItem const * SfxItemIter::GetCurItem()
+    if (m_nCurrent < m_nEnd)
-{
+    {
-    if (m_keys.empty())
+        do {
+            m_nCurrent++;
+        } while (m_nCurrent < m_nEnd && !*(ppFnd + m_nCurrent ));
+        return *(ppFnd+m_nCurrent);
+    }
     return nullptr;
-    auto it = m_rSet.m_aItems.find(*m_iter);
-    if (it == m_rSet.m_aItems.end())
-        return nullptr;
-    return it->second;
 }
 
-SfxPoolItem const * SfxItemIter::NextItem()
-{
-    if (m_iter == m_keys.end())
-        return nullptr;
-    ++m_iter;
-    if (m_iter == m_keys.end())
-        return nullptr;
-    return GetCurItem();
-}
-
-bool SfxItemIter::IsAtEnd() const
-{
-    return m_iter == m_keys.end() || std::next(m_iter) == m_keys.end();
-}
-
-
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */

svx/source/dialog/srchdlg.cxx

@@ -194,7 +194,7 @@ void SearchAttrItemList::Put( const SfxItemSet& rSet )
         // only test that it is available?
         if( IsInvalidItem( pItem ) )
         {
-            nWhich = aIter.GetCurWhich();
+            nWhich = rSet.GetWhichByPos( aIter.GetCurPos() );
             aItem.pItem = const_cast<SfxPoolItem*>(pItem);
         }
         else

sw/source/core/crsr/findattr.cxx

@@ -235,8 +235,8 @@ SwAttrCheckArr::SwAttrCheckArr( const SfxItemSet& rSet, bool bFwd,
 
     // determine area of Fnd/Stack array (Min/Max)
     SfxItemIter aIter( aCmpSet );
-    nArrStart = aIter.GetFirstWhich();
+    nArrStart = aCmpSet.GetWhichByPos( aIter.GetFirstPos() );
-    nArrLen = aIter.GetLastWhich() - nArrStart + 1;
+    nArrLen = aCmpSet.GetWhichByPos( aIter.GetLastPos() ) - nArrStart+1;
 
     char* pFndChar  = new char[ nArrLen * sizeof(SwSrchChrAttr) ];
     char* pStackChar = new char[ nArrLen * sizeof(SwSrchChrAttr) ];
@@ -287,7 +287,7 @@ void SwAttrCheckArr::SetNewSet( const SwTextNode& rTextNd, const SwPaM& rPam )
     {
         if( IsInvalidItem( pItem ) )
         {
-            nWhich = aIter.GetCurWhich();
+            nWhich = aCmpSet.GetWhichByPos( aIter.GetCurPos() );
             if( RES_TXTATR_END <= nWhich )
                 break; // end of text attributes
 
@@ -861,7 +861,7 @@ static bool lcl_Search( const SwContentNode& rCNd, const SfxItemSet& rCmpSet, bo
     {
         if( IsInvalidItem( pItem ))
         {
-            nWhich = aIter.GetCurWhich();
+            nWhich = rCmpSet.GetWhichByPos( aIter.GetCurPos() );
             if( SfxItemState::SET != rNdSet.GetItemState( nWhich, !bNoColls, &pNdItem )
                 || CmpAttr( *pNdItem, rNdSet.GetPool()->GetDefaultItem( nWhich ) ))
                 return false;

sw/source/core/undo/undobj1.cxx

@@ -531,7 +531,8 @@ void SwUndoSetFlyFormat::UndoImpl(::sw::UndoRedoContext & rContext)
         while( pItem )
         {
             if( IsInvalidItem( pItem ))
-                pFrameFormat->ResetFormatAttr( aIter.GetCurWhich() );
+                pFrameFormat->ResetFormatAttr( pItemSet->GetWhichByPos(
+                                        aIter.GetCurPos() ));
             else
                 pFrameFormat->SetFormatAttr( *pItem );
 

sw/source/uibase/app/docstyle.cxx

@@ -1677,7 +1677,8 @@ void SwDocStyleSheet::SetItemSet( const SfxItemSet& rSet,
             {
                 // use method <SwDoc::ResetAttrAtFormat(..)> in order to
                 // create an Undo object for the attribute reset.
-                rDoc.ResetAttrAtFormat( aIter.GetCurWhich(), *pFormat );
+                rDoc.ResetAttrAtFormat( rSet.GetWhichByPos(aIter.GetCurPos()),
+                                        *pFormat );
             }
 
             if( aIter.IsAtEnd() )