editeng/source/rtf/svxrtf.cxx | 47 - include/svl/itemiter.hxx | 49 - include/svl/itemset.hxx | 138 --- include/svl/whichranges.hxx | 12 include/svl/whiter.hxx | 31 sc/inc/patattr.hxx | 5 sc/source/core/data/patattr.cxx | 47 - sc/source/ui/view/cellsh3.cxx | 5 svl/qa/unit/items/stylepool.cxx | 11 svl/source/items/itemiter.cxx | 54 - svl/source/items/itemset.cxx | 1155 +++++++------------------- svl/source/items/whiter.cxx | 37 svx/source/dialog/srchdlg.cxx | 12 sw/source/core/crsr/findattr.cxx | 28 sw/source/core/doc/DocumentRedlineManager.cxx | 5 sw/source/core/doc/docfly.cxx | 34 sw/source/core/doc/docfmt.cxx | 5 sw/source/core/txtnode/ndtxt.cxx | 2 sw/source/core/txtnode/thints.cxx | 36 sw/source/core/undo/rolbck.cxx | 132 +- sw/source/core/undo/undobj1.cxx | 3 sw/source/filter/ww8/docxattributeoutput.cxx | 6 sw/source/filter/ww8/rtfattributeoutput.cxx | 1 sw/source/filter/ww8/writerhelper.cxx | 20 sw/source/filter/ww8/ww8atr.cxx | 58 + sw/source/filter/ww8/ww8attributeoutput.hxx | 2 sw/source/uibase/app/docstyle.cxx | 30 sw/source/uibase/uiview/srcview.cxx | 14 sw/source/uibase/wrtsh/wrtsh1.cxx | 20 29 files changed, 757 insertions(+), 1242 deletions(-)
New commits: commit 290c8f6e048fedf63437e3fdf629555ac89dd3ad Author: Armin Le Grand (allotropia) <[email protected]> AuthorDate: Wed May 1 19:34:36 2024 +0200 Commit: Armin Le Grand <[email protected]> CommitDate: Tue Jun 11 17:26:42 2024 +0200 ITEM: Change SfxItemSet to use unordered_set With all the changes done for Items we can now do deeper basic changes to the ItemSet itself with manageable risk. I already did https://gerrit.libreoffice.org/c/core/+/166455 aka 'ITEM: Add measurements for SfxItemSet usages' to get some statistical information about the fill/usage grade of the ItemSet's fixed PtrArray to SfxPoolItems, check that out to get an own picture. Those results show that an average usage is between some extremes ranging from 0% to 50%, but when using more checks and using multiple files/interactions/edits in all applications we end up with around typical 12%-19% of that array used, the rest is nullptr's. Thus I thought about one of the initial ideas of this series of changes (ITEM), to use a std::unordered_map (A) instead of that fixed array of SfxPoolItem Ptr's (B). Tthat again was for a complete type-based rewrite, which I once did a POC, but the code cannot be adapted to that, just too much work. Those are very different in architecture, (B) is done since a long time (since ever), but as pointed out above, (A) is now possible. There are many aspects to it, let's grep some: Speed (iterate): (A) and (B) are both linear. (A) has less entries, but may touch different mem areas (buckets). (B) is linear, but many empty spaces which are usually uselessly iterated. Speed (access Item by WhichID): (A) is hashed by WhichID, so mostly linear for unordered_set/hash. (B) is in a linear array, but has to calculate the offset for each WhichID access. So I guess speed will mostly equal out. Memory: (A) will be dynamically allocated (buckets), but stl is highly optimized and may even re-use areas, has to provide some extra info but will need less places for Items since it's dynamic and can start empty. (B) will be allocated once (except AllItemSet) and may even be 'derived' to the ItemSet (SfxItemSetFixed), but has to allocate all space at once. I can go in lots of more detail here, but due to the results of the statistics I just made a test now, including measuring some results (will include in gerrit, not here). I used two pro versions for that. That way I have some data now. Result is: - It is now possible to do this, it runs stable :-) - Speed: As expected, mostly no change - Memory: Depending on usage, 0% to 25% less, usually around 8-10% Side effects: - SfxAllItemSet could be done without WhichRanges, thus without expensive 'merges' - SfxItemSetFixed is not needed. While the idea is good, it needs a lot of extra stuff - Access to Items is linear if set - WhichRanges: Still needed, but for vaildity checking/filtering of ItemSet content - WhichRanges: Worth to think about if these are needed at all, probably just exist for historical reasons since allocation/number of added Items was never ever dynamic -> just not allocatable Putting the current version on gerrit, may still trigger some UTs (checked SW/SC/SD...) I did not like that functionality at ItemSet that hands out a vector of the set items for cases where to avoid iterating and deleting items at the same time at an ItemSet, so changed to using a local vector of remembered WhichIDs and deleting after the iterator is done. I also saw some strange usages of SfxItemIter in sw which i will have to check. Since there are still problems with UTs which I can not reproduce locally I have now added asserts to the case that an ItemSet gets changed with still having active SfxItemIter(s). That is always an error, but with the architecture of that fixed array of ItemPtrs did not have devastating effects (purely by coincidence). With that asserts, UTs run well in SC and SD, but in SW I get 11 (eleven!) asserts from the UTs, all of them from 'ITEM: SfxItemSet ClearItem' BTW. I guess these have to be fixed before thinking about this change... Good news is that all 11 cases were the same in SW, in SwHistorySetAttrSet::SwHistorySetAttrSet which does some strange things using two SfxItemIter in parallel. Thus SW UTs are also clear and I see no more errors caused by ItemSets being changed while SfxItemIters are alive. Bad news is that I still have errors to hunt... NOTE: Have now cleaned all UTs, this showed that there are some unexpected side-effects of the Items being processed in another order when SfxItemIter is used, also found one case where a WhichRangesContainer is constructed for a SfxItemSet using the set items from another ItemSet and SfxItemIter to do so. There *might* be more cases not covered by UTs. NOTE: While speed stays the same and mem is reduced up to 25% (see measurements in 1st comment) another *important* aspect is that this frees the way for using ItemSets *without* WhichRanges - these are necessary mainly to create that fixed array of pointers to the Items in a *manageable* size. With a dynamic structure like unordered_set there is in principle *no need* anymore to use WhichRanges to pre-define the Items a Set could hold. There is one exception: We have cases where one ItemSet is set at another one with defined WhichRanges to *filter* the contained Items - these would have to be identified. This is a rare case and we would have to check cases where an ItemSet gets set at another ItemSet. This would be as if all ItemSets would be AllItemSets in principle - much easier for everyone. NOTE: Waited for 24.8 split just to not take unnecessary risks. Change-Id: I75b0ac1d8a4495a3ee93c1117bdf618702785990 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166972 Reviewed-by: Armin Le Grand <[email protected]> Tested-by: Jenkins diff --git a/editeng/source/rtf/svxrtf.cxx b/editeng/source/rtf/svxrtf.cxx index f24b61b0b4f6..c14574877150 100644 --- a/editeng/source/rtf/svxrtf.cxx +++ b/editeng/source/rtf/svxrtf.cxx @@ -700,17 +700,22 @@ void SvxRTFParser::AttrGroupEnd() // process the current, delete from Stack // set only the attributes that are different from the parent if( pCurrent && pOld->aAttrSet.Count() ) { - SfxItemIter aIter( pOld->aAttrSet ); - const SfxPoolItem* pItem = aIter.GetCurItem(), *pGet; - do + // ITEM: SfxItemIter and removing SfxPoolItems: + // iterating and clearing Items on the same incarnation is in + // general a bad idea, it invalidates iterators. Work around + // this by remembering the WhichIDs of Items to delete + std::vector<sal_uInt16> aDeleteWhichIDs; + + for (SfxItemIter aIter(pOld->aAttrSet); !aIter.IsAtEnd(); aIter.NextItem()) { - if( SfxItemState::SET == pCurrent->aAttrSet.GetItemState( - pItem->Which(), false, &pGet ) && - *pItem == *pGet ) - aIter.ClearItem(); + const SfxPoolItem* pGet(nullptr); + if (SfxItemState::SET == pCurrent->aAttrSet.GetItemState(aIter.GetCurWhich(), false, &pGet) + && *aIter.GetCurItem() == *pGet) + aDeleteWhichIDs.push_back(aIter.GetCurWhich()); + } - pItem = aIter.NextItem(); - } while (pItem); + for (auto nDelWhich : aDeleteWhichIDs) + pOld->aAttrSet.ClearItem(nDelWhich); if (!pOld->aAttrSet.Count() && pOld->maChildList.empty() && !pOld->nStyleNo ) @@ -1102,17 +1107,19 @@ void SvxRTFItemStackType::Compress( const SvxRTFParser& rParser ) if( n ) { // Search for all which are set over the whole area - SfxItemIter aIter( aMrgSet ); - const SfxPoolItem* pItem; - const SfxPoolItem* pIterItem = aIter.GetCurItem(); - do { - sal_uInt16 nWhich = pIterItem->Which(); - if( SfxItemState::SET != pTmp->aAttrSet.GetItemState( nWhich, - false, &pItem ) || *pItem != *pIterItem) - aIter.ClearItem(); - - pIterItem = aIter.NextItem(); - } while(pIterItem); + // ITEM: SfxItemIter and removing SfxPoolItems: + std::vector<sal_uInt16> aDeleteWhichIDs; + + for (SfxItemIter aIter(aMrgSet); !aIter.IsAtEnd(); aIter.NextItem()) + { + const SfxPoolItem* pGet(nullptr); + if (SfxItemState::SET != pTmp->aAttrSet.GetItemState(aIter.GetCurWhich(), false, &pGet) + || *aIter.GetCurItem() != *pGet) + aDeleteWhichIDs.push_back(aIter.GetCurWhich()); + } + + for (auto nDelWhich : aDeleteWhichIDs) + aMrgSet.ClearItem(nDelWhich); if( !aMrgSet.Count() ) return; diff --git a/include/svl/itemiter.hxx b/include/svl/itemiter.hxx index 13cf14b61012..93f1c2a89655 100644 --- a/include/svl/itemiter.hxx +++ b/include/svl/itemiter.hxx @@ -27,32 +27,51 @@ class SfxPoolItem; class SVL_DLLPUBLIC SfxItemIter { const SfxItemSet& m_rSet; - sal_uInt16 m_nStart; - sal_uInt16 m_nEnd; - sal_uInt16 m_nCurrent; + PoolItemMap::const_iterator maCurrent; public: - SfxItemIter(const SfxItemSet& rSet); + SfxItemIter(const SfxItemSet& rSet) + : m_rSet(rSet) + , maCurrent(rSet.m_aPoolItemMap.begin()) + { +#ifdef DBG_UTIL + const_cast<SfxItemSet&>(m_rSet).m_nRegisteredSfxItemIter++; +#endif + } + +#ifdef DBG_UTIL + ~SfxItemIter() { const_cast<SfxItemSet&>(m_rSet).m_nRegisteredSfxItemIter--; } +#endif - /// get item, or null if no items const SfxPoolItem* GetCurItem() const { - return m_rSet.m_nCount ? *(m_rSet.m_ppItems + m_nCurrent) : nullptr; + if (IsAtEnd()) + return nullptr; + + return maCurrent->second; + } + + sal_uInt16 GetCurWhich() const + { + if (IsAtEnd()) + return 0; + + return maCurrent->first; } - const SfxPoolItem* NextItem() { return (m_nCurrent < m_nEnd) ? ImplNextItem() : nullptr; } - bool IsAtEnd() const { return m_nCurrent == m_nEnd; } + const SfxPoolItem* NextItem() + { + if (IsAtEnd()) + return nullptr; - sal_uInt16 GetCurPos() const { return m_nCurrent; } - sal_uInt16 GetFirstPos() const { return m_nStart; } - sal_uInt16 GetLastPos() const { return m_nEnd; } + maCurrent++; + return GetCurItem(); + } + + bool IsAtEnd() const { return maCurrent == m_rSet.m_aPoolItemMap.end(); } SfxItemState GetItemState(bool bSrchInParent = true, const SfxPoolItem** ppItem = nullptr) const; - void ClearItem(); - -private: - const SfxPoolItem* ImplNextItem(); }; #endif diff --git a/include/svl/itemset.hxx b/include/svl/itemset.hxx index 21b146ee393e..2a3ba3c915fa 100644 --- a/include/svl/itemset.hxx +++ b/include/svl/itemset.hxx @@ -23,6 +23,7 @@ #include <cassert> #include <memory> #include <utility> +#include <unordered_map> #include <svl/svldllapi.h> #include <svl/poolitem.hxx> @@ -42,7 +43,7 @@ SVL_DLLPUBLIC void listSfxItemSetUsage(); // ItemSet/ItemPool helpers SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource, bool bPassingOwnership); -void implCleanupItemEntry(SfxPoolItem const* pSource); +void implCleanupItemEntry(const SfxPoolItem* pSource); // coverity[ missing_move_assignment : SUPPRESS] - don't report about missing move assignment class SAL_WARN_UNUSED SVL_DLLPUBLIC SfxPoolItemHolder @@ -75,26 +76,31 @@ public: sal_uInt16 Which() const { if(nullptr != m_pItem) return m_pItem->Which(); return 0; } }; +typedef std::unordered_map<sal_uInt16, const SfxPoolItem*> PoolItemMap; + class SAL_WARN_UNUSED SVL_DLLPUBLIC SfxItemSet { friend class SfxItemIter; friend class SfxWhichIter; + friend class SfxAllItemSet; // allow ItemSetTooling to access friend SfxPoolItem const* implCreateItemEntry(SfxItemPool&, SfxPoolItem const*, bool); - friend void implCleanupItemEntry(SfxPoolItem const*); + friend void implCleanupItemEntry(const SfxPoolItem*); SfxItemPool* m_pPool; ///< pool that stores the items const SfxItemSet* m_pParent; ///< derivation - sal_uInt16 m_nCount; ///< number of items sal_uInt16 m_nRegister; ///< number of items with NeedsSurrogateSupport - // bitfield (better packaging if a bool needs to be added) - bool m_bItemsFixed : 1; ///< true if this is a SfxItemSetFixed object, so does not *own* m_ppItems +#ifdef DBG_UTIL + sal_uInt16 m_nRegisteredSfxItemIter; +#endif - SfxPoolItem const** m_ppItems; ///< pointer to array of items, we allocate and free this unless m_bItemsFixed==true WhichRangesContainer m_aWhichRanges; ///< array of Which Ranges + // the set SfxPoolItems, indexed by WhichID + PoolItemMap m_aPoolItemMap; + // Notification-Callback mechanism for SwAttrSet in SW, functionPtr for callback std::function<void(const SfxPoolItem*, const SfxPoolItem*)> m_aCallback; @@ -107,56 +113,26 @@ protected: void setCallback(const std::function<void(const SfxPoolItem*, const SfxPoolItem*)> &func) { m_aCallback = func; } void clearCallback() { m_aCallback = nullptr; } - // container library interface support - // only for internal use (for now), thus protected - using const_iterator = SfxPoolItem const**; - - const_iterator begin() const noexcept { return m_ppItems; } - const_iterator end() const noexcept { return begin() + TotalCount(); } - - bool empty() const noexcept { return 0 == TotalCount(); } - sal_Int32 size() const noexcept { return TotalCount(); } - SfxPoolItem const* operator[](sal_Int32 idx) const noexcept - { - assert(idx >= 0 && idx < TotalCount() && "index out of range"); - return m_ppItems[idx]; - } - -friend class SfxAllItemSet; - -private: - SVL_DLLPRIVATE void RecreateRanges_Impl(const WhichRangesContainer& pNewRanges); - -public: - SfxPoolItem const** GetItems_Impl() const { return m_ppItems; } - -private: - const SfxItemSet& operator=(const SfxItemSet &) = delete; - -protected: virtual const SfxPoolItem* PutImpl( const SfxPoolItem&, bool bPassingOwnership ); const SfxPoolItem* PutImplAsTargetWhich( const SfxPoolItem&, sal_uInt16 nTargetWhich, bool bPassingOwnership ); - /** special constructor for SfxAllItemSet */ - enum class SfxAllItemSetFlag { Flag }; - SfxItemSet( SfxItemPool&, SfxAllItemSetFlag ); - /** special constructor for SfxItemSetFixed */ - SfxItemSet( SfxItemPool&, WhichRangesContainer&& ranges, SfxPoolItem const ** ppItems); - /** special constructor for SfxItemSetFixed copy constructor */ - SfxItemSet( const SfxItemSet& rOther, SfxPoolItem const ** ppMyItems ); +private: + SVL_DLLPRIVATE void RecreateRanges_Impl(const WhichRangesContainer& pNewRanges); + const SfxItemSet& operator=(const SfxItemSet &) = delete; public: SfxItemSet( const SfxItemSet& ); SfxItemSet( SfxItemSet&& ) noexcept; SfxItemSet( SfxItemPool& ); SfxItemSet( SfxItemPool&, WhichRangesContainer ranges ); - SfxItemSet( SfxItemPool& rPool, sal_uInt16 nWhichStart, sal_uInt16 nWhichEnd ) : SfxItemSet(rPool, WhichRangesContainer(nWhichStart, nWhichEnd)) {} template<sal_uInt16... WIDs> - SfxItemSet(SfxItemPool& pool, svl::Items_t<WIDs...> wids) - : SfxItemSet(pool, WhichRangesContainer(wids)) {} + static SfxItemSet makeFixedSfxItemSet(SfxItemPool& pool) + { + return SfxItemSet(pool, WhichRangesContainer(svl::Items_t<WIDs...>{})); + } virtual ~SfxItemSet(); @@ -166,11 +142,9 @@ public: SfxItemSet CloneAsValue(bool bItems = true, SfxItemPool *pToPool = nullptr) const; // Get number of items - sal_uInt16 Count() const { return m_nCount; } + sal_uInt16 Count() const { return m_aPoolItemMap.size(); } sal_uInt16 TotalCount() const { return m_aWhichRanges.TotalCount(); } - bool IsItemsFixed() const { return m_bItemsFixed; } - const SfxPoolItem& Get( sal_uInt16 nWhich, bool bSrchInParent = true ) const; template<class T> const T& Get( TypedWhichId<T> nWhich, bool bSrchInParent = true ) const @@ -200,7 +174,6 @@ public: return GetItem<T>(sal_uInt16(nWhich), bSearchInParent); } - /// Templatized static version of GetItem() to directly return the correct type if the SfxItemSet is available. template<class T> static const T* GetItem(const SfxItemSet* pItemSet, sal_uInt16 nWhich, bool bSearchInParent) { @@ -216,12 +189,6 @@ public: return GetItem<T>(pItemSet, static_cast<sal_uInt16>(nWhich), bSearchInParent); } - sal_uInt16 GetWhichByOffset(sal_uInt16 nOffset) const; - - // Optimised variant for those situations where we know exactly what our which ranges are, - // and we can determine the offset at compile time. - const SfxPoolItem& GetByOffset(sal_uInt16 nWhich, sal_uInt16 nOffset) 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 @@ -256,10 +223,12 @@ public: { DisableOrInvalidateItem_ForWhichID(false, nWhich); } sal_uInt16 ClearItem( sal_uInt16 nWhich = 0); void ClearInvalidItems(); - inline void SetParent( const SfxItemSet* pNew ); + inline void SetParent( const SfxItemSet* pNew ) + { + m_pParent = pNew; + } // add, delete items, work on items -public: const SfxPoolItem* Put( const SfxPoolItem& rItem ) { return PutImpl(rItem, /*bPassingOwnership*/false); } const SfxPoolItem* Put( std::unique_ptr<SfxPoolItem> xItem ) @@ -306,35 +275,26 @@ public: void dumpAsXml(xmlTextWriterPtr pWriter) const; private: - // split version(s) of ClearSingleItemImpl for input types WhichID and Offset + // split version(s) of ClearSingleItemImpl for input type WhichID sal_uInt16 ClearSingleItem_ForWhichID( sal_uInt16 nWhich ); - sal_uInt16 ClearSingleItem_ForOffset( sal_uInt16 nOffset ); + void ClearSingleItem_PrepareRemove(const SfxPoolItem* pItem); // 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 bIgnoreDefaults); + void MergeItem_Impl(sal_uInt16 nWhich, const SfxPoolItem *pFnd2, bool bIgnoreDefaults); - // split version(s) of InvalidateItem/DisableItem for input types WhichID and Offset + // InvalidateItem/DisableItem for input types WhichID and Offset void DisableOrInvalidateItem_ForWhichID(bool bDsiable, sal_uInt16 nWhich); - void DisableOrInvalidateItem_ForOffset(bool bDisable, sal_uInt16 nOffset); - // split version(s) of GetItemStateImpl for input types WhichID and Offset + // GetItemStateImpl for input type WhichID SfxItemState GetItemState_ForWhichID( SfxItemState eState, sal_uInt16 nWhich, bool bSrchInParent, const SfxPoolItem **ppItem) const; - SfxItemState GetItemState_ForOffset( sal_uInt16 nOffset, const SfxPoolItem **ppItem) const; }; -inline void SfxItemSet::SetParent( const SfxItemSet* pNew ) -{ - m_pParent = pNew; -} - -class SVL_DLLPUBLIC SfxAllItemSet final : public SfxItemSet - // Handles all Ranges. Ranges are automatically modified by putting items. - +class SVL_DLLPUBLIC SfxAllItemSet final : public SfxItemSet { public: SfxAllItemSet( SfxItemPool &rPool ); @@ -347,41 +307,17 @@ private: }; -namespace svl::detail -{ -/** - * Determines the number of sal_uInt16s in a container of pairs of - * sal_uInt16s, each representing a range of sal_uInt16s, and total capacity of the ranges. - */ -template <sal_uInt16 WID1, sal_uInt16 WID2, sal_uInt16... Rest> -static constexpr sal_uInt16 CountRanges1() -{ - sal_uInt16 nCapacity = rangeSize(WID1, WID2); - if constexpr (sizeof...(Rest) > 0) - nCapacity += CountRanges1<Rest...>(); - return nCapacity; -}} - -// Split out the array because we need it to be initialised before we call -// the SfxItemSet constructor -template<sal_uInt16... WIDs> -struct SfxItemSetFixedStorage -{ - static constexpr sal_uInt16 NITEMS = svl::detail::CountRanges1<WIDs...>(); - const SfxPoolItem* m_aItems[NITEMS] {}; -}; - -// Allocate the items array inside the object, to reduce allocation cost. -// +// Was: Allocate the items array inside the object, to reduce allocation cost. +// NOTE: No longer needed with unordered_set, but there are 560+ places to +// replace/adapt, so keep it for now. +// To adapt, there is the static makeFixedSfxItemSet member in SfxItemSet. Did +// that in one place to check functionality (easy hack...?) template<sal_uInt16... WIDs> -class SfxItemSetFixed : public SfxItemSetFixedStorage<WIDs...>, public SfxItemSet +class SfxItemSetFixed : public SfxItemSet { public: SfxItemSetFixed( SfxItemPool& rPool) - : SfxItemSet(rPool, WhichRangesContainer(svl::Items_t<WIDs...>{}), - SfxItemSetFixedStorage<WIDs...>::m_aItems) {} - SfxItemSetFixed( const SfxItemSetFixed<WIDs...>& rOther ) - : SfxItemSet(rOther, SfxItemSetFixedStorage<WIDs...>::m_aItems) {} + : SfxItemSet(rPool, WhichRangesContainer(svl::Items_t<WIDs...>{})){}//, }; /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/include/svl/whichranges.hxx b/include/svl/whichranges.hxx index 0ed5e56835c4..53413d8c4c9e 100644 --- a/include/svl/whichranges.hxx +++ b/include/svl/whichranges.hxx @@ -86,7 +86,7 @@ class SVL_DLLPUBLIC WhichRangesContainer mutable sal_uInt16 m_TotalCount = 0; // variables for buffering the last used WhichPair to allow fast answers - // in getOffsetFromWhich + // in doesContainWhich mutable sal_uInt16 m_aLastWhichPairOffset = INVALID_WHICHPAIR_OFFSET; mutable sal_uInt16 m_aLastWhichPairFirst = 0; mutable sal_uInt16 m_aLastWhichPairSecond = 0; @@ -167,12 +167,10 @@ public: void reset(); - // calculate and return the offset inside the fixed SfxPoolItem - // array of SfxItemPool - sal_uInt16 getOffsetFromWhich(sal_uInt16 nWhich) const; - - // extract the WhichID for given offset - sal_uInt16 getWhichFromOffset(sal_uInt16 nOffset) const; + // calculate and return if nWhich is member of one of + // the local WhichRanges and such contained in the + // defined range + bool doesContainWhich(sal_uInt16 nWhich) const; // Adds a range to which ranges, keeping the ranges in valid state (sorted, non-overlapping) SAL_WARN_UNUSED_RESULT WhichRangesContainer MergeRange(sal_uInt16 nFrom, sal_uInt16 nTo) const; diff --git a/include/svl/whiter.hxx b/include/svl/whiter.hxx index 155d02d7f3ab..f83126a8ccdb 100644 --- a/include/svl/whiter.hxx +++ b/include/svl/whiter.hxx @@ -20,8 +20,8 @@ #include <svl/svldllapi.h> #include <svl/whichranges.hxx> +#include <svl/itemset.hxx> -class SfxItemSet; class SfxPoolItem; enum class SfxItemState; @@ -35,18 +35,35 @@ class SVL_DLLPUBLIC SfxWhichIter const SfxItemSet& m_rItemSet; const WhichPair* m_pCurrentWhichPair; sal_uInt16 m_nOffsetFromStartOfCurrentWhichPair; - /// Offset into m_ppItems array in SfxItemSet - sal_uInt16 m_nItemsOffset; public: - SfxWhichIter(const SfxItemSet& rSet); + SfxWhichIter(const SfxItemSet& rSet) + : m_rItemSet(rSet) + , m_pCurrentWhichPair(rSet.m_aWhichRanges.begin()) + , m_nOffsetFromStartOfCurrentWhichPair(0) + { + } sal_uInt16 GetCurWhich() const; sal_uInt16 NextWhich(); sal_uInt16 FirstWhich(); - SfxItemState GetItemState(bool bSrchInParent = true, - const SfxPoolItem** ppItem = nullptr) const; - void ClearItem(); + + SfxItemState GetItemState(bool bSrchInParent = true, const SfxPoolItem** ppItem = nullptr) const + { + const sal_uInt16 nWhich(GetCurWhich()); + if (0 == nWhich) + return SfxItemState::UNKNOWN; + return m_rItemSet.GetItemState_ForWhichID(SfxItemState::UNKNOWN, nWhich, bSrchInParent, + ppItem); + } + + void ClearItem() + { + const sal_uInt16 nWhich(GetCurWhich()); + if (0 == nWhich) + return; + const_cast<SfxItemSet&>(m_rItemSet).ClearSingleItem_ForWhichID(nWhich); + } }; /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sc/inc/patattr.hxx b/sc/inc/patattr.hxx index 5db0e8f9c82c..fb47d86c74a4 100644 --- a/sc/inc/patattr.hxx +++ b/sc/inc/patattr.hxx @@ -155,7 +155,10 @@ public: const SfxItemSet& GetItemSet() const { return maLocalSfxItemSet; } SfxItemSet& GetItemSet() { return maLocalSfxItemSet; } - const SfxPoolItem& GetItem(sal_uInt16 nWhichP) const { return maLocalSfxItemSet.GetByOffset(nWhichP, nWhichP - ATTR_PATTERN_START); } + const SfxPoolItem& GetItem(sal_uInt16 nWhichP) const + { + return maLocalSfxItemSet.Get(nWhichP, true); // GetByOffset used bSrchInParent==true + } template<class T> const T& GetItem( TypedWhichId<T> nWhich ) const { return static_cast<const T&>(GetItem(sal_uInt16(nWhich))); } static const SfxPoolItem& GetItem(sal_uInt16 nWhich, const SfxItemSet& rItemSet, const SfxItemSet* pCondSet); diff --git a/sc/source/core/data/patattr.cxx b/sc/source/core/data/patattr.cxx index 162e46d5de4a..5169ca412875 100644 --- a/sc/source/core/data/patattr.cxx +++ b/sc/source/core/data/patattr.cxx @@ -48,6 +48,7 @@ #include <svl/intitem.hxx> #include <svl/numformat.hxx> #include <svl/whiter.hxx> +#include <svl/itemiter.hxx> #include <svl/zforlist.hxx> #include <vcl/outdev.hxx> #include <tools/fract.hxx> @@ -444,8 +445,6 @@ static bool StrCmp( const OUString* pStr1, const OUString* pStr2 ) return *pStr1 == *pStr2; } -constexpr size_t compareSize = ATTR_PATTERN_END - ATTR_PATTERN_START + 1; - bool ScPatternAttr::operator==(const ScPatternAttr& rCmp) const { // check if same incarnation @@ -456,46 +455,14 @@ bool ScPatternAttr::operator==(const ScPatternAttr& rCmp) const if (!StrCmp(GetStyleName(), rCmp.GetStyleName())) return false; - // here we need to compare the SfxItemSet. We *know* that these are - // all simple (one range, same range) - const SfxItemSet& rSet1(maLocalSfxItemSet); - const SfxItemSet& rSet2(rCmp.maLocalSfxItemSet); - - // the former method 'FastEqualPatternSets' mentioned: + // use SfxItemSet::operator==, does the same as locally done here before, + // including pool compare (default). It also compares parent, not used here. + // There was the old comment from // "Actually test_tdf133629 from UITest_calc_tests9 somehow manages to have // a different range (and I don't understand enough why), so better be safe and compare fully." - // in that case the hash code above would already fail, too - assert(rSet1.TotalCount() == compareSize && rSet2.TotalCount() == compareSize); - - // check pools, do not accept different pools - if (rSet1.GetPool() != rSet2.GetPool()) - return false; - - // check count of set items, has to be equal - if (rSet1.Count() != rSet2.Count()) - return false; - - // both have no items, done - if (0 == rSet1.Count()) - return true; - - // compare each item separately - const SfxPoolItem **ppItem1(rSet1.GetItems_Impl()); - const SfxPoolItem **ppItem2(rSet2.GetItems_Impl()); - - // are all pointers the same? - if (0 == memcmp(ppItem1, ppItem2, compareSize * sizeof(ppItem1[0]))) - return true; - - for (sal_uInt16 nPos(0); nPos < compareSize; nPos++) - { - if (!SfxPoolItem::areSame(*ppItem1, *ppItem2)) - return false; - ++ppItem1; - ++ppItem2; - } - - return true; + // that hints to different WhichRanges, but WhichRanges are not compared in + // the std-operator (but the Items - if needed - as was here done locally) + return maLocalSfxItemSet == rCmp.maLocalSfxItemSet; } bool ScPatternAttr::areSame(const ScPatternAttr* pItem1, const ScPatternAttr* pItem2) diff --git a/sc/source/ui/view/cellsh3.cxx b/sc/source/ui/view/cellsh3.cxx index 6dba303ea028..c62500c7e124 100644 --- a/sc/source/ui/view/cellsh3.cxx +++ b/sc/source/ui/view/cellsh3.cxx @@ -471,10 +471,9 @@ void ScCellShell::Execute( SfxRequest& rReq ) { if ( pReqArgs != nullptr ) { - // set cell attribute without dialog: - - SfxItemSetFixed<ATTR_PATTERN_START, ATTR_PATTERN_END> aEmptySet( *pReqArgs->GetPool() ); + SfxItemSet aEmptySet(SfxItemSet::makeFixedSfxItemSet<ATTR_PATTERN_START, ATTR_PATTERN_END>(*pReqArgs->GetPool())); + // SfxItemSetFixed<ATTR_PATTERN_START, ATTR_PATTERN_END> aEmptySet( *pReqArgs->GetPool() ); SfxItemSetFixed<ATTR_PATTERN_START, ATTR_PATTERN_END> aNewSet( *pReqArgs->GetPool() ); diff --git a/svl/qa/unit/items/stylepool.cxx b/svl/qa/unit/items/stylepool.cxx index ec82dbd72852..6bee19881c89 100644 --- a/svl/qa/unit/items/stylepool.cxx +++ b/svl/qa/unit/items/stylepool.cxx @@ -100,17 +100,6 @@ CPPUNIT_TEST_FIXTURE(StylePoolTest, testIterationOrder) CPPUNIT_ASSERT(!pIter->getNext()); } } - -CPPUNIT_TEST_FIXTURE(StylePoolTest, testFixedItemSet) -{ - rtl::Reference<SfxItemPool> pPool = new SfxItemPool(u"test"_ustr); - pPool->registerItemInfoPackage(getItemInfoPackageTest()); - - SfxItemSetFixed<1, 2> aItemSet1(*pPool); - SfxItemSetFixed<1, 2> aItemSet2(aItemSet1); // test copy constructor - - assert(aItemSet2.IsItemsFixed()); -} } /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/svl/source/items/itemiter.cxx b/svl/source/items/itemiter.cxx index d8864c387edd..6eaa33dc5702 100644 --- a/svl/source/items/itemiter.cxx +++ b/svl/source/items/itemiter.cxx @@ -20,65 +20,25 @@ #include <svl/itemiter.hxx> #include <svl/itemset.hxx> -SfxItemIter::SfxItemIter(const SfxItemSet& rItemSet) - : m_rSet(rItemSet) -{ - if (!m_rSet.m_nCount) - { - m_nStart = 0; - m_nEnd = 0; - } - else - { - SfxPoolItem const** ppFnd = m_rSet.m_ppItems; - - // 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; -} - -// Precondition : m_nCurrent < m_nEnd -const SfxPoolItem* SfxItemIter::ImplNextItem() -{ - SfxPoolItem const** ppFnd = m_rSet.m_ppItems; - do - { - m_nCurrent++; - } while (m_nCurrent < m_nEnd && !*(ppFnd + m_nCurrent)); - return *(ppFnd + m_nCurrent); -} - SfxItemState SfxItemIter::GetItemState(bool bSrchInParent, const SfxPoolItem** ppItem) const { - // we have the offset, so use it to profit. It is always valid, so no need - // to check if smaller than TotalCount() - SfxItemState eState(m_rSet.GetItemState_ForOffset(m_nCurrent, ppItem)); + if (IsAtEnd()) + return SfxItemState::UNKNOWN; + + const sal_uInt16 nWhich(maCurrent->first); + SfxItemState eState( + m_rSet.GetItemState_ForWhichID(SfxItemState::UNKNOWN, nWhich, true, ppItem)); // search in parent? if (bSrchInParent && nullptr != m_rSet.GetParent() && (SfxItemState::UNKNOWN == eState || SfxItemState::DEFAULT == eState)) { // nOffset was only valid for *local* SfxItemSet, need to continue with WhichID - const sal_uInt16 nWhich(m_rSet.GetWhichByOffset(m_nCurrent)); + // const sal_uInt16 nWhich(m_rSet.GetWhichByOffset(m_nCurrent)); eState = m_rSet.GetParent()->GetItemState_ForWhichID(eState, nWhich, true, ppItem); } return eState; } -void SfxItemIter::ClearItem() -{ - // we have the offset, so use it to profit. It is always valid, so no need - // to check if smaller than TotalCount() - const_cast<SfxItemSet&>(m_rSet).ClearSingleItem_ForOffset(m_nCurrent); -} - /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx index 4b1207a697aa..334fe60e2a70 100644 --- a/svl/source/items/itemset.cxx +++ b/svl/source/items/itemset.cxx @@ -263,14 +263,15 @@ bool SfxPoolItemHolder::operator==(const SfxPoolItemHolder &rHolder) const * contain any Items with SlotIds as Which values. */ SfxItemSet::SfxItemSet(SfxItemPool& rPool) - : m_pPool(&rPool) - , m_pParent(nullptr) - , m_nCount(0) - , m_nRegister(0) - , m_bItemsFixed(false) - , m_ppItems(new const SfxPoolItem*[rPool.GetMergedIdRanges().TotalCount()]{}) - , m_aWhichRanges(rPool.GetMergedIdRanges()) - , m_aCallback() +: m_pPool(&rPool) +, m_pParent(nullptr) +, m_nRegister(0) +#ifdef DBG_UTIL +, m_nRegisteredSfxItemIter(0) +#endif +, m_aWhichRanges(rPool.GetMergedIdRanges()) +, m_aPoolItemMap() +, m_aCallback() { #ifdef DBG_UTIL nAllocatedSfxItemSetCount++; @@ -279,96 +280,99 @@ SfxItemSet::SfxItemSet(SfxItemPool& rPool) assert(m_aWhichRanges.validRanges2()); } -SfxItemSet::SfxItemSet( SfxItemPool& rPool, SfxAllItemSetFlag ) - : m_pPool(&rPool) - , m_pParent(nullptr) - , m_nCount(0) - , m_nRegister(0) - , m_bItemsFixed(false) - , m_ppItems(nullptr) - , m_aWhichRanges() - , m_aCallback() -{ +SfxItemSet::SfxItemSet(SfxItemPool& pool, WhichRangesContainer wids) +: m_pPool(&pool) +, m_pParent(nullptr) +, m_nRegister(0) #ifdef DBG_UTIL - nAllocatedSfxItemSetCount++; - nUsedSfxItemSetCount++; +, m_nRegisteredSfxItemIter(0) #endif -} - -/** special constructor for SfxItemSetFixed */ -SfxItemSet::SfxItemSet( SfxItemPool& rPool, WhichRangesContainer&& ranges, SfxPoolItem const ** ppItems)//, sal_uInt16 nTotalCount ) - : m_pPool(&rPool) - , m_pParent(nullptr) - , m_nCount(0) - , m_nRegister(0) - , m_bItemsFixed(true) - , m_ppItems(ppItems) - , m_aWhichRanges(std::move(ranges)) - , m_aCallback() +, m_aWhichRanges(std::move(wids)) +, m_aPoolItemMap() +, m_aCallback() { #ifdef DBG_UTIL nAllocatedSfxItemSetCount++; nUsedSfxItemSetCount++; #endif - assert(ppItems); - assert(m_aWhichRanges.size() > 0); + assert(m_aWhichRanges.TotalCount() != 0); assert(m_aWhichRanges.validRanges2()); } -/** special constructor for SfxItemSetFixed copy constructor */ -SfxItemSet::SfxItemSet( const SfxItemSet& rOther, - SfxPoolItem const ** ppMyItems ) - : m_pPool(rOther.m_pPool) - , m_pParent(rOther.m_pParent) - , m_nCount(rOther.m_nCount) - , m_nRegister(rOther.m_nRegister) - , m_bItemsFixed(true) - , m_ppItems(ppMyItems) - , m_aWhichRanges(rOther.m_aWhichRanges) - , m_aCallback(rOther.m_aCallback) +SfxItemSet::SfxItemSet( const SfxItemSet& rASet ) +: m_pPool( rASet.m_pPool ) +, m_pParent( rASet.m_pParent ) +, m_nRegister( rASet.m_nRegister ) +#ifdef DBG_UTIL +, m_nRegisteredSfxItemIter(0) +#endif +, m_aWhichRanges( rASet.m_aWhichRanges ) +, m_aPoolItemMap() +, m_aCallback(rASet.m_aCallback) { #ifdef DBG_UTIL nAllocatedSfxItemSetCount++; nUsedSfxItemSetCount++; #endif - assert(ppMyItems); - assert(m_aWhichRanges.size() > 0); - assert(m_aWhichRanges.validRanges2()); - - if (0 == rOther.Count()) + if (rASet.GetRanges().empty()) return; - // Copy attributes - SfxPoolItem const** ppDst(m_ppItems); - - for (const auto& rSource : rOther) - { - *ppDst = implCreateItemEntry(*GetPool(), rSource, false); - ppDst++; - } + for (const auto& rSource : rASet.m_aPoolItemMap) + m_aPoolItemMap[rSource.first] = implCreateItemEntry(*GetPool(), rSource.second, false); + assert(m_aWhichRanges.validRanges2()); if (0 != m_nRegister) GetPool()->registerItemSet(*this); } -SfxItemSet::SfxItemSet(SfxItemPool& pool, WhichRangesContainer wids) - : m_pPool(&pool) - , m_pParent(nullptr) - , m_nCount(0) - , m_nRegister(0) - , m_bItemsFixed(false) - , m_ppItems(new const SfxPoolItem*[wids.TotalCount()]{}) - , m_aWhichRanges(std::move(wids)) - , m_aCallback() +SfxItemSet::SfxItemSet(SfxItemSet&& rASet) noexcept +: m_pPool( rASet.m_pPool ) +, m_pParent( rASet.m_pParent ) +, m_nRegister( rASet.m_nRegister ) +#ifdef DBG_UTIL +, m_nRegisteredSfxItemIter(0) +#endif +, m_aWhichRanges( std::move(rASet.m_aWhichRanges) ) +, m_aPoolItemMap( std::move(rASet.m_aPoolItemMap) ) +, m_aCallback(rASet.m_aCallback) { #ifdef DBG_UTIL nAllocatedSfxItemSetCount++; nUsedSfxItemSetCount++; + assert(0 == rASet.m_nRegisteredSfxItemIter && "ITEM: SfxItemSet MOVE constructor with active SfxItemIters (!)"); #endif - assert(m_aWhichRanges.TotalCount() != 0); + // deregister if rASet is registered before ptrs vanish + if (0 != rASet.m_nRegister) + rASet.GetPool()->unregisterItemSet(rASet); + + // register if new set needs that + if (0 != m_nRegister) + GetPool()->registerItemSet(*this); + + // taking over ownership + rASet.m_pPool = nullptr; + rASet.m_pParent = nullptr; + rASet.m_nRegister = 0; + rASet.m_aWhichRanges.reset(); + rASet.m_aCallback = nullptr; + assert(m_aWhichRanges.validRanges2()); } +SfxItemSet::~SfxItemSet() +{ +#ifdef DBG_UTIL + nAllocatedSfxItemSetCount--; + addArrayUsage(Count(), TotalCount()); +#endif + // cleanup items. No std::fill needed, we are done with this ItemSet. + // the callback is not set in destructor, so no worries about that + ClearAllItemsImpl(); + + // for invariant-testing + m_aWhichRanges.reset(); +} + // Class that implements global Item sharing. It uses rtti to // associate every Item-derivation with a possible incarnation // of a DefaultItemInstanceManager. This is the default, it will @@ -532,16 +536,6 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS // just use/return nullptr return nullptr; - // if (IsInvalidItem(pSource)) - // // SfxItemState::INVALID aka INVALID_POOL_ITEM - // // just use pSource which equals INVALID_POOL_ITEM - // return pSource; - - // if (IsDisabledItem(pSource)) - // // SfxItemState::DISABLED aka DISABLED_POOL_ITEM - // // just use pSource which equals DISABLED_POOL_ITEM - // return pSource; - if (pSource->isStaticDefault()) // static default Items can just be used without RefCounting // NOTE: This now includes IsInvalidItem/IsDisabledItem @@ -595,46 +589,6 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS SfxItemPool* pMasterPool(rPool.GetMasterPool()); assert(nullptr != pMasterPool); - // // currently need to check if pSource is a default Item and - // // avoid it to leave the Pool. This is necessary since Pools - // // currently delete their default items hard, resetting RefCnt - // // and killing them. This might change after a Pool refinement. - // // For now, replace them with the local Pool default. It *might* - // // be even necessary to replace with a cloned non-default instance - // // if the defaults differ. - // // NOTE: Currently even some Pools have no 'real' StaticDefaults, - // // but these also get deleted (sigh) - // if (IsStaticDefaultItem(pSource)) - // { - // assert(!bPassingOwnership && "ITEM: PassingOwnership not possible combined with StaticDefault (!)"); - // const SfxPoolItem* pStatic(pTargetPool->GetPoolDefaultItem(pSource->Which())); - // if (nullptr != pStatic) - // { - // if (SfxPoolItem::areSame(pSource, pStatic)) - // pSource = pStatic; - // else - // { - // pSource = pSource->Clone(pMasterPool); - // bPassingOwnership = true; - // } - // } - // } - // else if (IsDefaultItem(pSource)) - // { - // assert(!bPassingOwnership && "ITEM: PassingOwnership not possible combined with DynamicDefault (!)"); - // const SfxPoolItem* pDynamic(pTargetPool->GetUserDefaultItem(pSource->Which())); - // if (nullptr != pDynamic) - // { - // if (SfxPoolItem::areSame(pSource, pDynamic)) - // pSource = pDynamic; - // else - // { - // pSource = pSource->Clone(pMasterPool); - // bPassingOwnership = true; - // } - // } - // } - // The Item itself is shareable when it is used/added at an instance // that RefCounts the Item, SfxItemPool or SfxPoolItemHolder. Try // to share items that are already shared @@ -721,22 +675,12 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS return pSource; } -void implCleanupItemEntry(SfxPoolItem const* pSource) +void implCleanupItemEntry(const SfxPoolItem* pSource) { if (nullptr == pSource) // no entry, done return; - // if (IsInvalidItem(pSource)) - // // SfxItemState::INVALID aka INVALID_POOL_ITEM - // // nothing to do for invalid item entries - // return; - - // if (IsDisabledItem(pSource)) - // // SfxItemState::INVALID aka DISABLED_POOL_ITEM - // // nothing to do for disabled item entries - // return; - if (pSource->isStaticDefault()) // static default Items can just be used without RefCounting // NOTE: This now includes IsInvalidItem/IsDisabledItem @@ -788,140 +732,52 @@ void implCleanupItemEntry(SfxPoolItem const* pSource) delete pSource; } -SfxItemSet::SfxItemSet( const SfxItemSet& rASet ) - : m_pPool( rASet.m_pPool ) - , m_pParent( rASet.m_pParent ) - , m_nCount( rASet.m_nCount ) - , m_nRegister( rASet.m_nRegister ) - , m_bItemsFixed(false) - , m_ppItems(nullptr) - , m_aWhichRanges( rASet.m_aWhichRanges ) - , m_aCallback(rASet.m_aCallback) +// Delete single Items or all Items (nWhich == 0) +sal_uInt16 SfxItemSet::ClearItem( sal_uInt16 nWhich ) { -#ifdef DBG_UTIL - nAllocatedSfxItemSetCount++; - nUsedSfxItemSetCount++; -#endif - if (rASet.GetRanges().empty()) - return; - - if (0 == rASet.Count()) - { - // no Items set in source ItemSet, allocate new array - // *plus* init to nullptr - m_ppItems = new const SfxPoolItem* [TotalCount()]{}; - return; - } - - // allocate new array (no need to initialize, will be done below) - m_ppItems = new const SfxPoolItem* [TotalCount()]; - - // Copy attributes - SfxPoolItem const** ppDst(m_ppItems); + if( !Count() ) + return 0; - for (const auto& rSource : rASet) - { - *ppDst = implCreateItemEntry(*GetPool(), rSource, false); - ppDst++; - } + if( nWhich ) + return ClearSingleItem_ForWhichID(nWhich); - assert(m_aWhichRanges.validRanges2()); - if (0 != m_nRegister) - GetPool()->registerItemSet(*this); + // clear all & reset to nullptr + return ClearAllItemsImpl(); } -SfxItemSet::SfxItemSet(SfxItemSet&& rASet) noexcept - : m_pPool( rASet.m_pPool ) - , m_pParent( rASet.m_pParent ) - , m_nCount( rASet.m_nCount ) - , m_nRegister( rASet.m_nRegister ) - , m_bItemsFixed(false) - , m_ppItems( rASet.m_ppItems ) - , m_aWhichRanges( std::move(rASet.m_aWhichRanges) ) - , m_aCallback(rASet.m_aCallback) +sal_uInt16 SfxItemSet::ClearSingleItem_ForWhichID( sal_uInt16 nWhich ) { -#ifdef DBG_UTIL - nAllocatedSfxItemSetCount++; - nUsedSfxItemSetCount++; -#endif - if (rASet.m_bItemsFixed) - { - // have to make a copy - m_ppItems = new const SfxPoolItem* [TotalCount()]; + PoolItemMap::iterator aHit(m_aPoolItemMap.find(nWhich)); - // can just copy the pointers, the ones in the original m_ppItems - // array will no longer be used/referenced (unused mem, but not lost, - // it's part of the ItemSet-derived object). - std::copy(rASet.m_ppItems, rASet.m_ppItems + TotalCount(), m_ppItems); - } - - // deregister if rASet is registered before ptrs vanish - if (0 != rASet.m_nRegister) - rASet.GetPool()->unregisterItemSet(rASet); - - // register if new set needs that - if (0 != m_nRegister) - GetPool()->registerItemSet(*this); - - // taking over ownership - rASet.m_pPool = nullptr; - rASet.m_pParent = nullptr; - rASet.m_nCount = 0; - rASet.m_nRegister = 0; - rASet.m_ppItems = nullptr; - rASet.m_aWhichRanges.reset(); - rASet.m_aCallback = nullptr; - - assert(m_aWhichRanges.validRanges2()); -} + if (aHit == m_aPoolItemMap.end()) + return 0; -SfxItemSet::~SfxItemSet() -{ #ifdef DBG_UTIL - nAllocatedSfxItemSetCount--; - addArrayUsage(Count(), TotalCount()); + assert(0 == m_nRegisteredSfxItemIter && "ITEM: SfxItemSet ClearItem with active SfxItemIters (!)"); #endif - // cleanup items. No std::fill needed, we are done with this ItemSet. - // the callback is not set in destructor, so no worries about that - ClearAllItemsImpl(); - - if (!m_bItemsFixed) - { - // free SfxPoolItem array - delete[] m_ppItems; - } - - // for invariant-testing - m_aWhichRanges.reset(); -} -// Delete single Items or all Items (nWhich == 0) -sal_uInt16 SfxItemSet::ClearItem( sal_uInt16 nWhich ) -{ - if( !Count() ) - return 0; - - if( nWhich ) - return ClearSingleItem_ForWhichID(nWhich); + ClearSingleItem_PrepareRemove(aHit->second); + m_aPoolItemMap.erase(aHit); - // clear all & reset to nullptr - const sal_uInt16 nRetval(ClearAllItemsImpl()); - std::fill(begin(), begin() + TotalCount(), nullptr); - return nRetval; + return 1; } -sal_uInt16 SfxItemSet::ClearSingleItem_ForWhichID( sal_uInt16 nWhich ) +void SfxItemSet::ClearSingleItem_PrepareRemove(const SfxPoolItem* pItem) { - const sal_uInt16 nOffset(GetRanges().getOffsetFromWhich(nWhich)); + if (nullptr == pItem) + return; - if (INVALID_WHICHPAIR_OFFSET != nOffset) + // Notification-Callback + if(m_aCallback) { - // found, continue with offset - return ClearSingleItem_ForOffset(nOffset); + m_aCallback(pItem, nullptr); } - // not found, return sal_uInt16 nDel = 0; - return 0; + // check register for remove + checkRemovePoolRegistration(pItem); + + // cleanup item & reset ptr + implCleanupItemEntry(pItem); } void SfxItemSet::checkRemovePoolRegistration(const SfxPoolItem* pItem) @@ -971,10 +827,6 @@ void SfxItemSet::checkAddPoolRegistration(const SfxPoolItem* pItem) // not needed for this item, done return; - // there cannot be more than m_nCount, *but* use one more to - // allow paired Remove/Add calls (see SfxItemSet::PutImpl) - assert(m_nRegister <= m_nCount); - // register when first Item that NeedsSurrogateSupport exist if (0 == m_nRegister) GetPool()->registerItemSet(*this); @@ -983,55 +835,23 @@ void SfxItemSet::checkAddPoolRegistration(const SfxPoolItem* pItem) m_nRegister++; } -sal_uInt16 SfxItemSet::ClearSingleItem_ForOffset( sal_uInt16 nOffset ) -{ - assert(nOffset < TotalCount()); - const_iterator aEntry(begin() + nOffset); - assert(nullptr == *aEntry || IsInvalidItem(*aEntry) || IsDisabledItem(*aEntry) || 0 != (*aEntry)->Which()); - - if (nullptr == *aEntry) - // no entry, done - return 0; - - // we reset an entry to nullptr -> decrement count - --m_nCount; - - // Notification-Callback - if(m_aCallback) - { - m_aCallback(*aEntry, nullptr); - } - - // check register for remove - checkRemovePoolRegistration(*aEntry); - - // cleanup item & reset ptr - implCleanupItemEntry(*aEntry); - *aEntry = nullptr; - - return 1; -} - sal_uInt16 SfxItemSet::ClearAllItemsImpl() { if (0 == Count()) // no items set, done return 0; - // loop & cleanup items - for (auto& rCandidate : *this) - { - if (nullptr != rCandidate && m_aCallback) - { - m_aCallback(rCandidate, nullptr); - } +#ifdef DBG_UTIL + assert(0 == m_nRegisteredSfxItemIter && "ITEM: SfxItemSet ClearAllItems with active SfxItemIters (!)"); +#endif - implCleanupItemEntry(rCandidate); - } + // loop & cleanup items + for (const auto& rCandidate : m_aPoolItemMap) + ClearSingleItem_PrepareRemove(rCandidate.second); // remember count before resetting it, that is the retval const sal_uInt16 nRetval(Count()); - m_nCount = 0; + m_aPoolItemMap.clear(); if (0 != m_nRegister) { @@ -1049,24 +869,46 @@ void SfxItemSet::ClearInvalidItems() return; // loop, here using const_iterator due to need to set ptr in m_ppItems array - for (const_iterator candidate(begin()); candidate != end(); candidate++) + for (PoolItemMap::iterator aCandidate(m_aPoolItemMap.begin()); aCandidate != m_aPoolItemMap.end();) { - if (IsInvalidItem(*candidate)) + if (IsInvalidItem(aCandidate->second)) { - *candidate = nullptr; - --m_nCount; +#ifdef DBG_UTIL + assert(0 == m_nRegisteredSfxItemIter && "ITEM: SfxItemSet ClearInvalidItems with active SfxItemIters (!)"); +#endif + aCandidate = m_aPoolItemMap.erase(aCandidate); } + else + aCandidate++; } } SfxItemState SfxItemSet::GetItemState_ForWhichID( SfxItemState eState, sal_uInt16 nWhich, bool bSrchInParent, const SfxPoolItem **ppItem) const { - const sal_uInt16 nOffset(GetRanges().getOffsetFromWhich(nWhich)); + PoolItemMap::const_iterator aHit(m_aPoolItemMap.find(nWhich)); + + if (aHit != m_aPoolItemMap.end()) + { + if (IsInvalidItem(aHit->second)) + // Different ones are present + return SfxItemState::INVALID; + + if (IsDisabledItem(aHit->second)) + // Item is Disabled + return SfxItemState::DISABLED; + + // if we have the Item, add it to output an hand back + if (nullptr != ppItem) + *ppItem = aHit->second; + + // Item is set + return SfxItemState::SET; + } - if (INVALID_WHICHPAIR_OFFSET != nOffset) + if (GetRanges().doesContainWhich(nWhich)) { - // found, continue with offset - eState = GetItemState_ForOffset(nOffset, ppItem); + // set to Default + eState = SfxItemState::DEFAULT; } // search in parent? @@ -1080,33 +922,6 @@ SfxItemState SfxItemSet::GetItemState_ForWhichID( SfxItemState eState, sal_uInt1 return eState; } -SfxItemState SfxItemSet::GetItemState_ForOffset( sal_uInt16 nOffset, const SfxPoolItem **ppItem) const -{ - // check and assert from invalid offset. The caller is responsible for - // ensuring a valid offset (see callers, all checked & safe) - assert(nOffset < TotalCount()); - SfxPoolItem const* pCandidate(*(begin() + nOffset)); - - if (nullptr == pCandidate) - // set to Default - return SfxItemState::DEFAULT; - - if (IsInvalidItem(pCandidate)) - // Different ones are present - return SfxItemState::INVALID; - - if (IsDisabledItem(pCandidate)) - // Item is Disabled - return SfxItemState::DISABLED; - - if (nullptr != ppItem) - // if we have the Item, add it to output an hand back - *ppItem = pCandidate; - - // Item is set - return SfxItemState::SET; -} - bool SfxItemSet::HasItem(sal_uInt16 nWhich, const SfxPoolItem** ppItem) const { const bool bRet(SfxItemState::SET == GetItemState_ForWhichID(SfxItemState::UNKNOWN, nWhich, true, ppItem)); @@ -1154,59 +969,59 @@ const SfxPoolItem* SfxItemSet::PutImpl(const SfxPoolItem& rItem, bool bPassingOw return nullptr; } -#ifdef _WIN32 - // Win build *insists* for initialization, triggers warning C4701: - // "potentially uninitialized local variable 'aEntry' used" for - // lines below. This is not the case here, but I know of no way - // to silence the warning in another way - const_iterator aEntry(begin()); -#else - const_iterator aEntry; -#endif + const sal_uInt16 nWhich(rItem.Which()); - sal_uInt16 nOffset(GetRanges().getOffsetFromWhich(rItem.Which())); - bool bActionNeeded(INVALID_WHICHPAIR_OFFSET != nOffset); + if (!GetRanges().doesContainWhich(nWhich)) + { + // no action needed: not in WhichRange + if (bPassingOwnership) + delete &rItem; + return nullptr; + } + + const SfxPoolItem* pEntry(nullptr); + PoolItemMap::iterator aHit(m_aPoolItemMap.find(nWhich)); - if (bActionNeeded) + if (aHit != m_aPoolItemMap.end()) { - aEntry = begin() + nOffset; + // compare items, evtl. containing content compare + pEntry = aHit->second; - if (nullptr == *aEntry) - { - // increase count if there was no entry before - ++m_nCount; - } - else + if (SfxPoolItem::areSame(*pEntry, rItem)) { - // compare items, evtl. containing content compare - bActionNeeded = !SfxPoolItem::areSame(**aEntry, rItem); + // no action needed: identical item already in place + if (bPassingOwnership) + delete &rItem; + return nullptr; } } - if (!bActionNeeded) - { - if (bPassingOwnership) - delete &rItem; - return nullptr; - } - // prepare new entry const SfxPoolItem* pNew(implCreateItemEntry(*GetPool(), &rItem, bPassingOwnership)); // Notification-Callback if(m_aCallback) { - m_aCallback(*aEntry, pNew); + m_aCallback(pEntry, pNew); } // check register for add/remove. add first so that unregister/register // is avoided when an Item is replaced (increase, decrease, do not reach 0) checkAddPoolRegistration(pNew); - checkRemovePoolRegistration(*aEntry); + checkRemovePoolRegistration(pEntry); // cleanup old entry & set entry at m_ppItems array - implCleanupItemEntry(*aEntry); - *aEntry = pNew; + implCleanupItemEntry(pEntry); + + if (pEntry) + aHit->second = pNew; + else + { +#ifdef DBG_UTIL + assert(0 == m_nRegisteredSfxItemIter && "ITEM: SfxItemSet PutImpl with active SfxItemIters (!)"); +#endif + m_aPoolItemMap[nWhich] = pNew; + } return pNew; } @@ -1217,42 +1032,25 @@ bool SfxItemSet::Put(const SfxItemSet& rSource, bool bInvalidAsDefault) // no items in source, done return false; - const_iterator aSource(rSource.begin()); - sal_uInt16 nNumberToGo(rSource.Count()); bool bRetval(false); - // iterate based on WhichIDs to have it available for evtl. PutImpl calls - for (const WhichPair& rPair : rSource.GetRanges()) + for (PoolItemMap::const_iterator aCandidate(rSource.m_aPoolItemMap.begin()); aCandidate != rSource.m_aPoolItemMap.end(); aCandidate++) { - for (sal_uInt16 nWhich(rPair.first); nWhich <= rPair.second; nWhich++, aSource++) + if (IsInvalidItem(aCandidate->second)) { - if (nullptr != *aSource) + if (bInvalidAsDefault) { - nNumberToGo--; - - if (IsInvalidItem(*aSource)) - { - if (bInvalidAsDefault) - { - bRetval |= 0 != ClearSingleItem_ForWhichID(nWhich); - } - else - { - DisableOrInvalidateItem_ForWhichID(false, nWhich); - } - } - else - { - bRetval |= nullptr != PutImpl(**aSource, false); - } + bRetval |= 0 != ClearSingleItem_ForWhichID(aCandidate->first); } - - if (0 == nNumberToGo) + else { - // we can return early if all set Items are handled - return bRetval; + DisableOrInvalidateItem_ForWhichID(false, aCandidate->first); } } + else + { + bRetval |= nullptr != PutImpl(*aCandidate->second, false); + } } return bRetval; @@ -1281,15 +1079,15 @@ void SfxItemSet::PutExtended ) { // don't "optimize" with "if( rSource.Count()" because of dontcare + defaults - const_iterator aSource(rSource.begin()); - for (const WhichPair& rPair : rSource.GetRanges()) { - for (sal_uInt16 nWhich = rPair.first; nWhich <= rPair.second; nWhich++, aSource++) + for (sal_uInt16 nWhich = rPair.first; nWhich <= rPair.second; nWhich++) { - if (nullptr != *aSource) + PoolItemMap::const_iterator aHit(rSource.m_aPoolItemMap.find(nWhich)); + + if (aHit != rSource.m_aPoolItemMap.end()) { - if (IsInvalidItem(*aSource)) + if (IsInvalidItem(aHit->second)) { // Item is DontCare: switch (eDontCareAs) @@ -1313,7 +1111,7 @@ void SfxItemSet::PutExtended else { // Item is set: - PutImpl(**aSource, false); + PutImpl(*aHit->second, false); } } else @@ -1355,7 +1153,7 @@ void SfxItemSet::MergeRange( sal_uInt16 nFrom, sal_uInt16 nTo ) bool bAllIncluded(!GetRanges().empty()); for (sal_uInt16 a(nFrom); bAllIncluded && a <= nTo; a++) - if (INVALID_WHICHPAIR_OFFSET == GetRanges().getOffsetFromWhich(a)) + if (!GetRanges().doesContainWhich(a)) bAllIncluded = false; // if yes, we are done @@ -1396,72 +1194,25 @@ void SfxItemSet::SetRanges( WhichRangesContainer&& aNewRanges ) void SfxItemSet::RecreateRanges_Impl(const WhichRangesContainer& rNewRanges) { - // create new item-array (by iterating through all new ranges) - const sal_uInt16 nNewTotalCount(rNewRanges.TotalCount()); - SfxPoolItem const** aNewItemArray(new const SfxPoolItem* [nNewTotalCount]); - sal_uInt16 nNewCount(0); - if (0 == Count()) - { - // no Items set, just reset to nullptr (default) - std::fill(aNewItemArray, aNewItemArray + nNewTotalCount, nullptr); - } - else - { - // iterate over target - all entries there need to be set/initialized. Use - // WhichIDs, we need to access two different WhichRanges - const_iterator aNewTarget(aNewItemArray); - - for (auto const & rNewRange : rNewRanges) - { - for (sal_uInt16 nWhich(rNewRange.first); nWhich <= rNewRange.second; nWhich++, aNewTarget++) - { - // check if we have that WhichID in source ranges - const sal_uInt16 nSourceOffset(GetRanges().getOffsetFromWhich(nWhich)); - - if (INVALID_WHICHPAIR_OFFSET == nSourceOffset) - { - // no entry in source, init to nullptr - *aNewTarget = nullptr; - } - else - { - // we have entry in source, transfer entry - whatever it may be, - // also for nullptr (for initialization) - const_iterator aSourceEntry(begin() + nSourceOffset); - *aNewTarget = *aSourceEntry; - - // take care of new Count - if (nullptr != *aNewTarget) - { - nNewCount++; - } - - // reset entry, since it got transferred it should not be cleaned-up - *aSourceEntry = nullptr; - } - } - } + // no existing items, done + return; - // free old items: only necessary when not all Items were transferred - if (nNewCount != Count()) + // check if existing items are in the new ItemRanges. + // if they are not, remove the item + for (PoolItemMap::iterator aCandidate(m_aPoolItemMap.begin()); aCandidate != m_aPoolItemMap.end();) + { + if (!rNewRanges.doesContainWhich(aCandidate->first)) { - ClearAllItemsImpl(); +#ifdef DBG_UTIL + assert(0 == m_nRegisteredSfxItemIter && "ITEM: SfxItemSet RecreateRanges with active SfxItemIters (!)"); +#endif + ClearSingleItem_PrepareRemove(aCandidate->second); + aCandidate = m_aPoolItemMap.erase(aCandidate); } + else + aCandidate++; } - - // replace old items-array and ranges - if (m_bItemsFixed) - { - m_bItemsFixed = false; - } - else - { - delete[] m_ppItems; - } - - m_ppItems = aNewItemArray; - m_nCount = nNewCount; } /** @@ -1499,38 +1250,37 @@ bool SfxItemSet::Set rSet are not taken into account */ ) { - bool bRet = false; if (Count()) ClearItem(); - if ( bDeep ) + + if (!bDeep) + return Put(rSet, false); + + bool bRet = false; + SfxWhichIter aIter1(*this); + SfxWhichIter aIter2(rSet); + sal_uInt16 nWhich1 = aIter1.FirstWhich(); + sal_uInt16 nWhich2 = aIter2.FirstWhich(); + for (;;) { - SfxWhichIter aIter1(*this); - SfxWhichIter aIter2(rSet); - sal_uInt16 nWhich1 = aIter1.FirstWhich(); - sal_uInt16 nWhich2 = aIter2.FirstWhich(); - for (;;) + if (!nWhich1 || !nWhich2) + break; + if (nWhich1 > nWhich2) { - if (!nWhich1 || !nWhich2) - break; - if (nWhich1 > nWhich2) - { - nWhich2 = aIter2.NextWhich(); - continue; - } - if (nWhich1 < nWhich2) - { - nWhich1 = aIter1.NextWhich(); - continue; - } - const SfxPoolItem* pItem; - if( SfxItemState::SET == aIter2.GetItemState( true, &pItem ) ) - bRet |= nullptr != Put( *pItem ); - nWhich1 = aIter1.NextWhich(); nWhich2 = aIter2.NextWhich(); + continue; } + if (nWhich1 < nWhich2) + { + nWhich1 = aIter1.NextWhich(); + continue; + } + const SfxPoolItem* pItem; + if( SfxItemState::SET == aIter2.GetItemState( true, &pItem ) ) + bRet |= nullptr != Put( *pItem ); + nWhich1 = aIter1.NextWhich(); + nWhich2 = aIter2.NextWhich(); } - else - bRet = Put(rSet, false); return bRet; } @@ -1554,25 +1304,19 @@ const SfxPoolItem* SfxItemSet::GetItem(sal_uInt16 nId, bool bSearchInParent) con const SfxPoolItem& SfxItemSet::Get( sal_uInt16 nWhich, bool bSrchInParent) const { - // Search the Range in which the Which is located in: - const sal_uInt16 nOffset(GetRanges().getOffsetFromWhich(nWhich)); + PoolItemMap::const_iterator aHit(m_aPoolItemMap.find(nWhich)); - if (INVALID_WHICHPAIR_OFFSET != nOffset) + if (aHit != m_aPoolItemMap.end()) { - const_iterator aFoundOne(begin() + nOffset); - - if (nullptr != *aFoundOne) + if (IsInvalidItem(aHit->second)) { - if (IsInvalidItem(*aFoundOne)) - { - return GetPool()->GetUserOrPoolDefaultItem(nWhich); - } + return GetPool()->GetUserOrPoolDefaultItem(nWhich); + } #ifdef DBG_UTIL - if (IsDisabledItem(*aFoundOne)) - SAL_INFO("svl.items", "SFX_WARNING: Getting disabled Item"); + if (IsDisabledItem(aHit->second)) + SAL_INFO("svl.items", "SFX_WARNING: Getting disabled Item"); #endif - return **aFoundOne; - } + return *aHit->second; } if (bSrchInParent && nullptr != GetParent()) @@ -1617,33 +1361,18 @@ void SfxItemSet::Intersect( const SfxItemSet& rSet ) // locally delete all Items that are *not* set in rSet // -> != SfxItemState::SET - // same ranges? - if (GetRanges() == rSet.GetRanges()) - { - for (sal_uInt16 nOffset(0); nOffset < TotalCount(); nOffset++) - { - if (SfxItemState::SET != rSet.GetItemState_ForOffset(nOffset, nullptr)) - { - // can use same offset - ClearSingleItem_ForOffset(nOffset); - } - } - } - else + for (PoolItemMap::iterator aCandidate(m_aPoolItemMap.begin()); aCandidate != m_aPoolItemMap.end();) { - sal_uInt16 nOffset(0); - - for (auto const & rRange : GetRanges()) + if (SfxItemState::SET != rSet.GetItemState_ForWhichID(SfxItemState::UNKNOWN, aCandidate->first, false, nullptr)) { - for (sal_uInt16 nWhich(rRange.first); nWhich <= rRange.second; nWhich++, nOffset++) - { - if (SfxItemState::SET != rSet.GetItemState_ForWhichID(SfxItemState::UNKNOWN, nWhich, false, nullptr)) - { - // can use offset - ClearSingleItem_ForOffset(nOffset); - } - } +#ifdef DBG_UTIL + assert(0 == m_nRegisteredSfxItemIter && "ITEM: SfxItemSet Intersect with active SfxItemIters (!)"); +#endif + ClearSingleItem_PrepareRemove(aCandidate->second); + aCandidate = m_aPoolItemMap.erase(aCandidate); } + else + aCandidate++; } } @@ -1673,33 +1402,18 @@ void SfxItemSet::Differentiate(const SfxItemSet& rSet) // locally delete all Items that *are *not* set in rSet // -> ==SfxItemState::SET - // same ranges? - if (GetRanges() == rSet.GetRanges()) + for (PoolItemMap::iterator aCandidate(m_aPoolItemMap.begin()); aCandidate != m_aPoolItemMap.end();) { - for (sal_uInt16 nOffset(0); nOffset < TotalCount(); nOffset++) + if (SfxItemState::SET == rSet.GetItemState_ForWhichID(SfxItemState::UNKNOWN, aCandidate->first, false, nullptr)) { - if (SfxItemState::SET == rSet.GetItemState_ForOffset(nOffset, nullptr)) - { - // can use same offset - ClearSingleItem_ForOffset(nOffset); - } - } - } - else - { - sal_uInt16 nOffset(0); - - for (auto const & rRange : GetRanges()) - { - for (sal_uInt16 nWhich(rRange.first); nWhich <= rRange.second; nWhich++, nOffset++) - { - if (SfxItemState::SET == rSet.GetItemState_ForWhichID(SfxItemState::UNKNOWN, nWhich, false, nullptr)) - { - // can use offset - ClearSingleItem_ForOffset(nOffset); - } - } +#ifdef DBG_UTIL + assert(0 == m_nRegisteredSfxItemIter && "ITEM: SfxItemSet Differentiate with active SfxItemIters (!)"); +#endif + ClearSingleItem_PrepareRemove(aCandidate->second); + aCandidate = m_aPoolItemMap.erase(aCandidate); } + else + aCandidate++; } } @@ -1781,77 +1495,83 @@ void SfxItemSet::Differentiate(const SfxItemSet& rSet) * dontcare unknown != sal_True - - - * unknown unknown != sal_True - - - */ -void SfxItemSet::MergeItem_Impl(const SfxPoolItem **ppFnd1, const SfxPoolItem *pFnd2, bool bIgnoreDefaults) + +void SfxItemSet::MergeItem_Impl(sal_uInt16 nWhich, const SfxPoolItem *pFnd2, bool bIgnoreDefaults) { - assert(ppFnd1 != nullptr && "Merging to 0-Item"); + // callers need to ensure that nWhich is in local range + assert(GetRanges().doesContainWhich(nWhich) && "ITEM: call to MergeItem_Impl with WhichID outside local range (!)"); + const PoolItemMap::iterator aHit(m_aPoolItemMap.find(nWhich)); - // 1st Item is Default? - if ( !*ppFnd1 ) + if (aHit == m_aPoolItemMap.end()) { - if ( IsInvalidItem(pFnd2) ) + // 1st Item nWhich is not set (Default) + const SfxPoolItem* pNew(nullptr); + + if (IsInvalidItem(pFnd2)) // Decision table: default, dontcare, doesn't matter, doesn't matter - *ppFnd1 = INVALID_POOL_ITEM; + pNew = INVALID_POOL_ITEM; - else if ( pFnd2 && !bIgnoreDefaults && - GetPool()->GetUserOrPoolDefaultItem(pFnd2->Which()) != *pFnd2 ) + else if (pFnd2 && !bIgnoreDefaults && GetPool()->GetUserOrPoolDefaultItem(nWhich) != *pFnd2) // Decision table: default, set, !=, sal_False - *ppFnd1 = INVALID_POOL_ITEM; + pNew = INVALID_POOL_ITEM; - else if ( pFnd2 && bIgnoreDefaults ) + else if (pFnd2 && bIgnoreDefaults) // Decision table: default, set, doesn't matter, sal_True - *ppFnd1 = implCreateItemEntry(*GetPool(), pFnd2, false); - // *ppFnd1 = &GetPool()->Put( *pFnd2 ); + pNew = implCreateItemEntry(*GetPool(), pFnd2, false); - if ( *ppFnd1 ) + if (pNew) { - ++m_nCount; - checkAddPoolRegistration(*ppFnd1); +#ifdef DBG_UTIL + assert(0 == m_nRegisteredSfxItemIter && "ITEM: SfxItemSet MergeItem with active SfxItemIters (!)"); +#endif + m_aPoolItemMap[nWhich] = pNew; + checkAddPoolRegistration(pNew); } + + return; } - // 1st Item set? - else if ( !IsInvalidItem(*ppFnd1) ) + const SfxPoolItem* pFnd1(aHit->second); + + if (IsInvalidItem(pFnd1)) { - if ( !pFnd2 ) - { - // 2nd Item is Default - if ( !bIgnoreDefaults && - **ppFnd1 != GetPool()->GetUserOrPoolDefaultItem((*ppFnd1)->Which()) ) - { - // Decision table: set, default, !=, sal_False - checkRemovePoolRegistration(*ppFnd1); - implCleanupItemEntry(*ppFnd1); - // GetPool()->Remove( **ppFnd1 ); - *ppFnd1 = INVALID_POOL_ITEM; - } - } - else if ( IsInvalidItem(pFnd2) ) + return; + } + + // 1st Item is set, check for change + bool bDoChange(false); + + if (nullptr == pFnd2) + { + // 2nd Item is not set (Default) + if (!bIgnoreDefaults && *pFnd1 != GetPool()->GetUserOrPoolDefaultItem(nWhich)) { - // 2nd Item is dontcare - if ( !bIgnoreDefaults || - **ppFnd1 != GetPool()->GetUserOrPoolDefaultItem( (*ppFnd1)->Which()) ) - { - // Decision table: set, dontcare, doesn't matter, sal_False - // or: set, dontcare, !=, sal_True - checkRemovePoolRegistration(*ppFnd1); - implCleanupItemEntry(*ppFnd1); - // GetPool()->Remove( **ppFnd1 ); - *ppFnd1 = INVALID_POOL_ITEM; - } + // Decision table: set, default, !=, sal_False + bDoChange = true; } - else + } + else if (IsInvalidItem(pFnd2)) + { + // 2nd Item is invaid (dontcare) + if (!bIgnoreDefaults || *pFnd1 != GetPool()->GetUserOrPoolDefaultItem(nWhich)) { - // 2nd Item is set - if ( **ppFnd1 != *pFnd2 ) - { - // Decision table: set, set, !=, doesn't matter - checkRemovePoolRegistration(*ppFnd1); - implCleanupItemEntry(*ppFnd1); - // GetPool()->Remove( **ppFnd1 ); - *ppFnd1 = INVALID_POOL_ITEM; - } + // Decision table: set, dontcare, doesn't matter, sal_False + // or: set, dontcare, !=, sal_True + bDoChange = true; } } + else if (*pFnd1 != *pFnd2) + { + // 2nd Item is set + // Decision table: set, set, !=, doesn't matter + bDoChange = true; + } + + if (bDoChange) + { + ClearSingleItem_PrepareRemove(pFnd1); + aHit->second = INVALID_POOL_ITEM; + } } void SfxItemSet::MergeValues( const SfxItemSet& rSet ) @@ -1869,33 +1589,13 @@ void SfxItemSet::MergeValues( const SfxItemSet& rSet ) // evtl. could not find that WhichID in local WhichRanges // Better to loop over local WhichRanges (these get changed) and look // for Item with same WhichID in rSet, this is done now. - if (GetRanges() == rSet.GetRanges()) - { - - // loop over both & merge, WhichIDs are identical - for (const_iterator dst(begin()), src(rSet.begin()); dst != end(); dst++, src++) - { - MergeItem_Impl(dst, *src, false/*bIgnoreDefaults*/); - } - } - else + for (auto const & rRange : GetRanges()) { - // loop over local which-ranges - the local Items need to be changed - const_iterator dst(begin()); - - for (auto const & rRange : GetRanges()) + for (sal_uInt16 nWhich(rRange.first); nWhich <= rRange.second; nWhich++) { - for (sal_uInt16 nWhich(rRange.first); nWhich <= rRange.second; nWhich++, dst++) - { - // try to get offset in rSet for same WhichID - const sal_uInt16 nOffset(rSet.GetRanges().getOffsetFromWhich(nWhich)); - - if (INVALID_WHICHPAIR_OFFSET != nOffset) - { - // if entry with same WhichID exists in rSet, merge with local entry - MergeItem_Impl(dst, *(rSet.begin() + nOffset), false/*bIgnoreDefaults*/); - } - } + PoolItemMap::const_iterator aHit(rSet.m_aPoolItemMap.find(nWhich)); + const SfxPoolItem* src(aHit == rSet.m_aPoolItemMap.end() ? nullptr : aHit->second); + MergeItem_Impl(nWhich, src, false/*bIgnoreDefaults*/); } } } @@ -1903,106 +1603,42 @@ void SfxItemSet::MergeValues( const SfxItemSet& rSet ) void SfxItemSet::MergeValue(const SfxPoolItem& rAttr) { if (IsDisabledItem(&rAttr)) - // is IsDisabledItem, nothing to do + // DisabledItem, nothing to do return; - const sal_uInt16 nOffset(GetRanges().getOffsetFromWhich(rAttr.Which())); - - if (INVALID_WHICHPAIR_OFFSET != nOffset) + if (GetRanges().doesContainWhich(rAttr.Which())) { - MergeItem_Impl(begin() + nOffset, &rAttr, /*bIgnoreDefaults*/true); + MergeItem_Impl(rAttr.Which(), &rAttr, /*bIgnoreDefaults*/true); } } void SfxItemSet::DisableOrInvalidateItem_ForWhichID(bool bDisable, sal_uInt16 nWhich) { - const sal_uInt16 nOffset(GetRanges().getOffsetFromWhich(nWhich)); + PoolItemMap::iterator aHit(m_aPoolItemMap.find(nWhich)); - if (INVALID_WHICHPAIR_OFFSET != nOffset) + if (aHit != m_aPoolItemMap.end()) { - DisableOrInvalidateItem_ForOffset(bDisable, nOffset); - } -} - -void SfxItemSet::DisableOrInvalidateItem_ForOffset(bool bDisable, sal_uInt16 nOffset) -{ - // check and assert from invalid offset. The caller is responsible for - // ensuring a valid offset (see callers, all checked & safe) - assert(nOffset < TotalCount()); - const_iterator aFoundOne(begin() + nOffset); - - if (nullptr == *aFoundOne) - { - // entry goes from nullptr -> invalid - ++m_nCount; - } - else - { - if (bDisable && IsDisabledItem(*aFoundOne)) + if (bDisable && IsDisabledItem(aHit->second)) // already disabled item, done return; - if (!bDisable && IsInvalidItem(*aFoundOne)) + if (!bDisable && IsInvalidItem(aHit->second)) // already invalid item, done return; - // cleanup entry - checkRemovePoolRegistration(*aFoundOne); - implCleanupItemEntry(*aFoundOne); + ClearSingleItem_PrepareRemove(aHit->second); + aHit->second = bDisable ? DISABLED_POOL_ITEM : INVALID_POOL_ITEM; } - - // set new entry - *aFoundOne = bDisable ? DISABLED_POOL_ITEM : INVALID_POOL_ITEM; -} - -sal_uInt16 SfxItemSet::GetWhichByOffset( sal_uInt16 nOffset ) const -{ - assert(nOffset < TotalCount()); - - // 1st try to get a set SfxPoolItem and fetch the WhichID from there. - const SfxPoolItem* pItem(nullptr); - (void)GetItemState_ForOffset(nOffset, &pItem); - - if (nullptr != pItem && 0 != pItem->Which()) - return pItem->Which(); - - // 2nd have to get from WhichRangesContainer. That might use - // the buffering, too. We might assert a return value of zero - // (which means invalid WhichID), but we already assert for - // a valid offset at the start of this method - return GetRanges().getWhichFromOffset(nOffset); -} - -const SfxPoolItem& SfxItemSet::GetByOffset( sal_uInt16 nWhich, sal_uInt16 nOffset ) const -{ - assert(nOffset < TotalCount()); - - const_iterator aFoundOne(begin() + nOffset); - - if (nullptr != *aFoundOne) + else if (GetRanges().doesContainWhich(nWhich)) { - if (IsInvalidItem(*aFoundOne)) - { - return GetPool()->GetUserOrPoolDefaultItem(nWhich); - } #ifdef DBG_UTIL - if (IsDisabledItem(*aFoundOne)) - SAL_INFO("svl.items", "SFX_WARNING: Getting disabled Item"); + assert(0 == m_nRegisteredSfxItemIter && "ITEM: SfxItemSet DisableOrInvalidateItem with active SfxItemIters (!)"); #endif - return **aFoundOne; - } - - if (nullptr != GetParent()) - { - return GetParent()->Get(nWhich, /*bSrchInParent*/true); + // new entry + m_aPoolItemMap[nWhich] = bDisable ? DISABLED_POOL_ITEM : INVALID_POOL_ITEM; } - - // Get the Default from the Pool and return - assert(m_pPool); - return GetPool()->GetUserOrPoolDefaultItem(nWhich); } - bool SfxItemSet::operator==(const SfxItemSet &rCmp) const { return Equals( rCmp, true); @@ -2030,66 +1666,20 @@ bool SfxItemSet::Equals(const SfxItemSet &rCmp, bool bComparePool) const if (0 == Count()) return true; - // check if ranges are equal - if (GetRanges() == rCmp.GetRanges()) + for (PoolItemMap::const_iterator aCandidate(m_aPoolItemMap.begin()); aCandidate != m_aPoolItemMap.end(); aCandidate++) { - // if yes, we can simplify: are all pointers the same? - if (0 == memcmp( m_ppItems, rCmp.m_ppItems, TotalCount() * sizeof(m_ppItems[0]) )) - return true; - - // compare each one separately - const SfxPoolItem **ppItem1(m_ppItems); - const SfxPoolItem **ppItem2(rCmp.m_ppItems); - - for (sal_uInt16 nPos(0); nPos < TotalCount(); nPos++) - { - // do full SfxPoolItem compare - if (!SfxPoolItem::areSame(*ppItem1, *ppItem2)) - return false; - ++ppItem1; - ++ppItem2; - } - - return true; - } + const SfxPoolItem *pItem1(nullptr); + const SfxPoolItem *pItem2(nullptr); + const sal_uInt16 nWhich(aCandidate->first); + const SfxItemState aStateA(GetItemState_ForWhichID(SfxItemState::UNKNOWN, nWhich, false, &pItem1)); + const SfxItemState aStateB(rCmp.GetItemState_ForWhichID(SfxItemState::UNKNOWN, nWhich, false, &pItem2)); - // Not same ranges, need to compare content. Only need to check if - // the content of one is inside the other due to already having - // compared Count() above. - // Iterate over local SfxItemSet by using locval ranges and offset, - // so we can access needed data at least for one SfxItemSet more - // direct. For the 2nd one we need the WhichID which we have by - // iterating over the ranges. - sal_uInt16 nOffset(0); - sal_uInt16 nNumberToGo(Count()); + if (aStateA != aStateB) + return false; - for (auto const & rRange : GetRanges()) - { - for (sal_uInt16 nWhich(rRange.first); nWhich <= rRange.second; nWhich++, nOffset++) - { - const SfxPoolItem *pItem1(nullptr); - const SfxPoolItem *pItem2(nullptr); - const SfxItemState aStateA(GetItemState_ForOffset(nOffset, &pItem1)); - const SfxItemState aStateB(rCmp.GetItemState_ForWhichID(SfxItemState::UNKNOWN, nWhich, false, &pItem2)); - - if (aStateA != aStateB) - return false; - - // only compare items if SfxItemState::SET, else the item ptrs are not set - if (SfxItemState::SET == aStateA && !SfxPoolItem::areSame(pItem1, pItem2)) - return false; - - if (SfxItemState::DEFAULT != aStateA) - // if local item is not-nullptr we have compared one more, reduce NumberToGo - // NOTE: we could also use 'nullptr != *(begin() + nOffset)' here, but the - // entry was already checked by GetItemState_ForOffset above - nNumberToGo--; - - if (0 == nNumberToGo) - // if we have compared Count() number of items and are still here - // (all were equal), we can exit early - return true; - } + // only compare items if SfxItemState::SET, else the item ptrs are not set + if (SfxItemState::SET == aStateA && !SfxPoolItem::areSame(pItem1, pItem2)) + return false; } return true; @@ -2172,7 +1762,7 @@ void SfxItemSet::dumpAsXml(xmlTextWriterPtr pWriter) const // ----------------------------------------------- class SfxAllItemSet SfxAllItemSet::SfxAllItemSet( SfxItemPool &rPool ) -: SfxItemSet(rPool, SfxAllItemSetFlag::Flag) +: SfxItemSet(rPool) { } @@ -2344,20 +1934,20 @@ static void isMiss() } #endif -sal_uInt16 WhichRangesContainer::getOffsetFromWhich(sal_uInt16 nWhich) const +bool WhichRangesContainer::doesContainWhich(sal_uInt16 nWhich) const { // special case for single entry - happens often e.g. UI stuff if (m_size == 1) { if( m_pairs->first <= nWhich && nWhich <= m_pairs->second ) - return nWhich - m_pairs->first; + return true; // we have only one WhichPair entry and it's not contained -> failed - return INVALID_WHICHPAIR_OFFSET; + return false; } if (m_size == 0) - return INVALID_WHICHPAIR_OFFSET; + return false; // check if nWhich is inside last successfully used WhichPair if (INVALID_WHICHPAIR_OFFSET != m_aLastWhichPairOffset @@ -2368,7 +1958,7 @@ sal_uInt16 WhichRangesContainer::getOffsetFromWhich(sal_uInt16 nWhich) const isHit(); #endif // we can re-use the last found WhichPair - return m_aLastWhichPairOffset + (nWhich - m_aLastWhichPairFirst); + return true; } #ifdef DBG_UTIL @@ -2389,7 +1979,7 @@ sal_uInt16 WhichRangesContainer::getOffsetFromWhich(sal_uInt16 nWhich) const m_aLastWhichPairSecond = rPair.second; // ...and return - return m_aLastWhichPairOffset + (nWhich - m_aLastWhichPairFirst); + return true; } m_aLastWhichPairOffset += rPair.second - rPair.first + 1; @@ -2399,62 +1989,7 @@ sal_uInt16 WhichRangesContainer::getOffsetFromWhich(sal_uInt16 nWhich) const // what could wrongly trigger re-use above for next search m_aLastWhichPairOffset = INVALID_WHICHPAIR_OFFSET; - return m_aLastWhichPairOffset; -} - -sal_uInt16 WhichRangesContainer::getWhichFromOffset(sal_uInt16 nOffset) const -{ - // special case for single entry - happens often e.g. UI stuff - if (m_size == 1) - { - if (nOffset <= m_pairs->second - m_pairs->first) - return m_pairs->first + nOffset; - - // we have only one WhichPair entry and it's not contained -> failed - return 0; - } - - // check for empty, if yes, return null which is an invalid WhichID - if (m_size == 0) - return 0; - - // check if nWhich is inside last successfully used WhichPair - if (INVALID_WHICHPAIR_OFFSET != m_aLastWhichPairOffset) - { - // only try if we are beyond or at m_aLastWhichPairOffset to - // not get numerically negative - if (nOffset >= m_aLastWhichPairOffset) - { - const sal_uInt16 nAdaptedOffset(nOffset - m_aLastWhichPairOffset); - - if (nAdaptedOffset <= m_aLastWhichPairSecond - m_aLastWhichPairFirst) - { -#ifdef DBG_UTIL - isHit(); -#endif - return m_aLastWhichPairFirst + nAdaptedOffset; - } - } - } - -#ifdef DBG_UTIL - isMiss(); -#endif - - // Iterate over WhichPairs in WhichRangesContainer - // Do not update buffered last hit (m_aLastWhichPair*), these calls - // are potentially more rare than getOffsetFromWhich calls. Still, - // it could also be done here - for( auto const & pPtr : *this ) - { - const sal_uInt16 nWhichPairRange(pPtr.second - pPtr.first); - if( nOffset <= nWhichPairRange ) - return pPtr.first + nOffset; - nOffset -= nWhichPairRange + 1; - } - - // no WhichID found, return invalid one - return 0; + return false; } // Adds a range to which ranges, keeping the ranges in valid state (sorted, non-overlapping) diff --git a/svl/source/items/whiter.cxx b/svl/source/items/whiter.cxx index 33fd59f9378d..e61f8fede51b 100644 --- a/svl/source/items/whiter.cxx +++ b/svl/source/items/whiter.cxx @@ -20,14 +20,6 @@ #include <svl/itemset.hxx> #include <svl/whiter.hxx> -SfxWhichIter::SfxWhichIter(const SfxItemSet& rSet) - : m_rItemSet(rSet) - , m_pCurrentWhichPair(rSet.m_aWhichRanges.begin()) - , m_nOffsetFromStartOfCurrentWhichPair(0) - , m_nItemsOffset(0) -{ -} - sal_uInt16 SfxWhichIter::GetCurWhich() const { const WhichRangesContainer& rWhichRanges = m_rItemSet.m_aWhichRanges; @@ -46,7 +38,6 @@ sal_uInt16 SfxWhichIter::NextWhich() ++m_nOffsetFromStartOfCurrentWhichPair; if (m_pCurrentWhichPair->second == nLastWhich) { - m_nItemsOffset += m_pCurrentWhichPair->second - m_pCurrentWhichPair->first + 1; ++m_pCurrentWhichPair; m_nOffsetFromStartOfCurrentWhichPair = 0; } @@ -59,35 +50,7 @@ sal_uInt16 SfxWhichIter::FirstWhich() { m_pCurrentWhichPair = m_rItemSet.m_aWhichRanges.begin(); m_nOffsetFromStartOfCurrentWhichPair = 0; - m_nItemsOffset = 0; return m_pCurrentWhichPair->first; } -SfxItemState SfxWhichIter::GetItemState(bool bSrchInParent, const SfxPoolItem** ppItem) const -{ - const sal_uInt16 nOffset(m_nItemsOffset + m_nOffsetFromStartOfCurrentWhichPair); - - // we have the offset, so use it to profit. It is always valid, so no need - // to check if smaller than TotalCount() - SfxItemState eState(m_rItemSet.GetItemState_ForOffset(nOffset, ppItem)); - - // search in parent? - if (bSrchInParent && nullptr != m_rItemSet.GetParent() && (SfxItemState::UNKNOWN == eState || SfxItemState::DEFAULT == eState)) - { - // nOffset was only valid for *local* SfxItemSet, need to continue with WhichID - // Use the *highest* SfxItemState as result - const sal_uInt16 nWhich(m_pCurrentWhichPair->first + m_nOffsetFromStartOfCurrentWhichPair); - return m_rItemSet.GetParent()->GetItemState_ForWhichID( eState, nWhich, true, ppItem); - } - - return eState; -} - -void SfxWhichIter::ClearItem() -{ - // we have the offset, so use it to profit. It is always valid, so no need - // to check if smaller than TotalCount() - const_cast<SfxItemSet&>(m_rItemSet).ClearSingleItem_ForOffset(m_nItemsOffset + m_nOffsetFromStartOfCurrentWhichPair); -} - /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/svx/source/dialog/srchdlg.cxx b/svx/source/dialog/srchdlg.cxx index bb66ecde8343..a1455a49e6d5 100644 --- a/svx/source/dialog/srchdlg.cxx +++ b/svx/source/dialog/srchdlg.cxx @@ -198,17 +198,13 @@ void SearchAttrItemList::Put( const SfxItemSet& rSet ) return; SfxItemPool* pPool = rSet.GetPool(); - SfxItemIter aIter( rSet ); - const SfxPoolItem* pItem = aIter.GetCurItem(); - do + for (SfxItemIter aIter(rSet); !aIter.IsAtEnd(); aIter.NextItem()) { - const sal_uInt16 nWhich(rSet.GetWhichByOffset(aIter.GetCurPos())); + const sal_uInt16 nWhich(aIter.GetCurWhich()); const sal_uInt16 nSlot(pPool->GetSlotId(nWhich)); - - emplace_back(nSlot, SfxPoolItemHolder(*pPool, pItem)); - pItem = aIter.NextItem(); - } while (pItem); + emplace_back(nSlot, SfxPoolItemHolder(*pPool, aIter.GetCurItem())); + } } diff --git a/sw/source/core/crsr/findattr.cxx b/sw/source/core/crsr/findattr.cxx index 7c4932f745d0..f4ff66f81e1b 100644 --- a/sw/source/core/crsr/findattr.cxx +++ b/sw/source/core/crsr/findattr.cxx @@ -220,9 +220,27 @@ SwAttrCheckArr::SwAttrCheckArr( const SfxItemSet& rSet, bool bFwd, m_aComapeSet.Put( rSet, false ); // determine area of Fnd/Stack array (Min/Max) - SfxItemIter aIter( m_aComapeSet ); - m_nArrStart = m_aComapeSet.GetWhichByOffset( aIter.GetFirstPos() ); - m_nArrLen = m_aComapeSet.GetWhichByOffset( aIter.GetLastPos() ) - m_nArrStart+1; + sal_uInt16 nMinUsedWhichID(0); + sal_uInt16 nMaxUsedWhichID(0); + + if (0 != m_aComapeSet.Count()) + { + nMinUsedWhichID = 5000; // SFX_WHICH_MAX+1; + for (SfxItemIter aIter(m_aComapeSet); !aIter.IsAtEnd(); aIter.NextItem()) + { + const sal_uInt16 nCurrentWhich(aIter.GetCurWhich()); + if (SfxItemPool::IsSlot(nCurrentWhich)) + continue; + nMinUsedWhichID = std::min(nMinUsedWhichID, nCurrentWhich); + nMaxUsedWhichID = std::max(nMaxUsedWhichID, nCurrentWhich); + } + + if (nMinUsedWhichID > nMaxUsedWhichID) + nMinUsedWhichID = nMaxUsedWhichID = 0; + } + + m_nArrStart = nMinUsedWhichID;//m_aComapeSet.GetWhichByOffset( aIter.GetFirstPos() ); + m_nArrLen = nMaxUsedWhichID - nMinUsedWhichID + 1;//m_aComapeSet.GetWhichByOffset( aIter.GetLastPos() ) - m_nArrStart+1; char* pFndChar = new char[ m_nArrLen * sizeof(SwSrchChrAttr) ]; char* pStackChar = new char[ m_nArrLen * sizeof(SwSrchChrAttr) ]; @@ -273,7 +291,7 @@ void SwAttrCheckArr::SetNewSet( const SwTextNode& rTextNd, const SwPaM& rPam ) { if( IsInvalidItem( pItem ) ) { - nWhich = m_aComapeSet.GetWhichByOffset( aIter.GetCurPos() ); + nWhich = aIter.GetCurWhich(); if( RES_TXTATR_END <= nWhich ) break; // end of text attributes @@ -887,7 +905,7 @@ static bool lcl_Search( const SwContentNode& rCNd, const SfxItemSet& rCmpSet, bo { if( IsInvalidItem( pItem )) { - nWhich = rCmpSet.GetWhichByOffset( aIter.GetCurPos() ); + nWhich = aIter.GetCurWhich(); if( SfxItemState::SET != rNdSet.GetItemState( nWhich, !bNoColls, &pNdItem ) || CmpAttr( *pNdItem, rNdSet.GetPool()->GetUserOrPoolDefaultItem( nWhich ) )) return false; diff --git a/sw/source/core/doc/DocumentRedlineManager.cxx b/sw/source/core/doc/DocumentRedlineManager.cxx index 9ae003980701..8be40de4c498 100644 --- a/sw/source/core/doc/DocumentRedlineManager.cxx +++ b/sw/source/core/doc/DocumentRedlineManager.cxx @@ -42,6 +42,7 @@ #include <osl/diagnose.h> #include <editeng/prntitem.hxx> #include <comphelper/lok.hxx> +#include <svl/itemiter.hxx> using namespace com::sun::star; @@ -442,9 +443,9 @@ namespace if (!bSameSet) { - for( sal_uInt16 nItem = 0; nItem < aTmp.TotalCount(); ++nItem) + for (SfxItemIter aIter(aTmp); !aIter.IsAtEnd(); aIter.NextItem()) { - sal_uInt16 nWhich = aTmp.GetWhichByOffset(nItem); + const sal_uInt16 nWhich(aIter.GetCurWhich()); if( SfxItemState::SET == aTmp.GetItemState( nWhich, false ) && SfxItemState::SET != aTmp2.GetItemState( nWhich, false ) ) aTmp2.Put( aTmp.GetPool()->GetUserOrPoolDefaultItem(nWhich) ); diff --git a/sw/source/core/doc/docfly.cxx b/sw/source/core/doc/docfly.cxx index 203894123007..52d9d601dc90 100644 --- a/sw/source/core/doc/docfly.cxx +++ b/sw/source/core/doc/docfly.cxx @@ -454,12 +454,13 @@ lcl_SetFlyFrameAttr(SwDoc & rDoc, ? (rDoc.*pSetFlyFrameAnchor)( rFlyFormat, rSet, false ) : DONTMAKEFRMS; - const SfxPoolItem* pItem; - SfxItemIter aIter( rSet ); - SfxItemSet aTmpSet( rDoc.GetAttrPool(), aFrameFormatSetRange ); - const SfxPoolItem* pItemIter = aIter.GetCurItem(); - do { - switch(pItemIter->Which()) + // ITEM: SfxItemIter and removing SfxPoolItems: + std::vector<sal_uInt16> aDeleteWhichIDs; + SfxItemSet aTmpSet(rDoc.GetAttrPool(), aFrameFormatSetRange); + + for (SfxItemIter aIter(rSet); !aIter.IsAtEnd() && 0 != aIter.GetCurWhich(); aIter.NextItem()) + { + switch(aIter.GetCurWhich()) { case RES_FILL_ORDER: case RES_BREAK: @@ -469,23 +470,26 @@ lcl_SetFlyFrameAttr(SwDoc & rDoc, OSL_FAIL( "Unknown Fly attribute." ); [[fallthrough]]; case RES_CHAIN: - rSet.ClearItem(pItemIter->Which()); + aDeleteWhichIDs.push_back(aIter.GetCurWhich()); break; case RES_ANCHOR: if( DONTMAKEFRMS != nMakeFrames ) break; [[fallthrough]]; default: - if( !IsInvalidItem(pItemIter) && ( SfxItemState::SET != - rFlyFormat.GetAttrSet().GetItemState(pItemIter->Which(), true, &pItem ) || - *pItem != *pItemIter)) - aTmpSet.Put(*pItemIter); - break; + { + const SfxPoolItem* pGet(nullptr); + if (!IsInvalidItem(aIter.GetCurItem()) && (SfxItemState::SET != + rFlyFormat.GetAttrSet().GetItemState(aIter.GetCurWhich(), true, &pGet ) || + *pGet != *aIter.GetCurItem())) + aTmpSet.Put(*aIter.GetCurItem()); + break; + } } + } - pItemIter = aIter.NextItem(); - - } while (pItemIter && (0 != pItemIter->Which())); + for (auto nDelWhich : aDeleteWhichIDs) + rSet.ClearItem(nDelWhich); if( aTmpSet.Count() ) rFlyFormat.SetFormatAttr( aTmpSet ); diff --git a/sw/source/core/doc/docfmt.cxx b/sw/source/core/doc/docfmt.cxx index 722d3dba8c0d..b25ae850d022 100644 --- a/sw/source/core/doc/docfmt.cxx +++ b/sw/source/core/doc/docfmt.cxx @@ -1885,6 +1885,11 @@ void SwDoc::SetFormatItemByAutoFormat( const SwPaM& rPam, const SfxItemSet& rSet { whichIds.push_back({pItem->Which(), pItem->Which()}); } + + // ITEM: Need to sort these due to ItemSet using unordered_set and + // WhichRangesContainer expect these sorted ascending + std::sort(whichIds.begin(), whichIds.end(), [](WhichPair& a, WhichPair& b) {return a.first < b.first;}); + SfxItemSet currentSet(GetAttrPool(), WhichRangesContainer(whichIds.data(), whichIds.size())); pTNd->GetParaAttr(currentSet, nEnd, nEnd); for (const WhichPair& rPair : whichIds) diff --git a/sw/source/core/txtnode/ndtxt.cxx b/sw/source/core/txtnode/ndtxt.cxx index 4cb8534ab205..c71bdce6bfb8 100644 --- a/sw/source/core/txtnode/ndtxt.cxx +++ b/sw/source/core/txtnode/ndtxt.cxx @@ -2588,7 +2588,7 @@ void SwTextNode::CutImpl( SwTextNode * const pDest, const SwContentIndex & rDest { // check current item const sal_uInt16 nWhich = IsInvalidItem( pItem ) - ? pDest->GetpSwAttrSet()->GetWhichByOffset( aIter.GetCurPos() ) + ? aIter.GetCurWhich() : pItem->Which(); if( RES_FRMATR_STYLE_NAME != nWhich && RES_FRMATR_CONDITIONAL_STYLE_NAME != nWhich && diff --git a/sw/source/core/txtnode/thints.cxx b/sw/source/core/txtnode/thints.cxx index 729a5443b03b..4aae2af56eb8 100644 --- a/sw/source/core/txtnode/thints.cxx +++ b/sw/source/core/txtnode/thints.cxx @@ -891,22 +891,23 @@ void SwpHints::BuildPortions( SwTextNode& rNode, SwTextAttr& rNewHint, // #i81764# This should not be applied for no length attributes!!! <-- if ( !bNoLengthAttribute && rNode.HasSwAttrSet() && aNewSet.Count() ) { - SfxItemIter aIter2( aNewSet ); - const SfxPoolItem* pItem = aIter2.GetCurItem(); - const SfxItemSet& rWholeParaAttrSet = rNode.GetSwAttrSet(); + const SfxItemSet& rWholeParaAttrSet(rNode.GetSwAttrSet()); + std::vector<sal_uInt16> aDeleteWhichIDs; - do + for (SfxItemIter aIter(aNewSet); !aIter.IsAtEnd(); aIter.NextItem()) { - const SfxPoolItem* pTmpItem = nullptr; - if ( SfxItemState::SET == rWholeParaAttrSet.GetItemState( pItem->Which(), false, &pTmpItem ) && - SfxPoolItem::areSame(pTmpItem, pItem) ) + const SfxPoolItem* pGet(nullptr); + if (SfxItemState::SET == rWholeParaAttrSet.GetItemState(aIter.GetCurWhich(), false, &pGet) && + SfxPoolItem::areSame(pGet, aIter.GetCurItem())) { // Do not clear item if the attribute is set in a character format: - if ( !pCurrentCharFormat || nullptr == CharFormat::GetItem( *pCurrentCharFormat, pItem->Which() ) ) - aIter2.ClearItem(); + if (!pCurrentCharFormat || nullptr == CharFormat::GetItem(*pCurrentCharFormat, aIter.GetCurWhich())) + aDeleteWhichIDs.push_back(aIter.GetCurWhich()); } } - while ((pItem = aIter2.NextItem())); + + for (auto nDelWhich : aDeleteWhichIDs) + aNewSet.ClearItem(nDelWhich); } // Remove old hint @@ -2404,15 +2405,20 @@ struct RemovePresentAttrs } const SwTextAttr* const pAutoStyle(i_rAttrSpan.second); - SfxItemIter aIter(m_rAttrSet); -e ... etc. - the rest is truncated
