improve the SfxItemSet::CloneAsValue check

to prevent object slicing.

Which reveals a problems with
commit 044fa30a4c77013c87a7e2a6dd9022a2f6599778
Author: Noel Grandin <noelgrandin@gmail.com>
Date:   Thu Sep 23 18:44:42 2021 +0200
no need to allocate this SfxItemSet on the heap
and
commit 044fa30a4c77013c87a7e2a6dd9022a2f6599778
Author: Noel Grandin <noelgrandin@gmail.com>
Date:   Thu Sep 23 18:44:42 2021 +0200
no need to allocate this SfxItemSet on the heap
so revert the problematic bits of those commits

Change-Id: I5eeba1d5bdb91f4e539850516f2b1c11e69ff2c1
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/130127
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

3 files changed, 17 insertions, 21 deletions

diff --git a/include/svl/itemset.hxx b/include/svl/itemset.hxx
index 5ce13bb1f4c8..8d5b9c3bb601 100644
--- a/include/svl/itemset.hxx
+++ b/include/svl/itemset.hxx
@@ -88,7 +88,9 @@ public:
     virtual ~SfxItemSet();
 
     virtual std::unique_ptr<SfxItemSet> Clone(bool bItems = true, SfxItemPool *pToPool = nullptr) const;
-    virtual SfxItemSet CloneAsValue(bool bItems = true, SfxItemPool *pToPool = nullptr) const;
+    /** note that this only works if you know for sure that you are dealing with an SfxItemSet
+        and not one of it's subclasses. */
+    SfxItemSet CloneAsValue(bool bItems = true, SfxItemPool *pToPool = nullptr) const;
 
     // Get number of items
     sal_uInt16                  Count() const { return m_nCount; }
@@ -219,7 +221,6 @@ public:
                                 SfxAllItemSet( const SfxAllItemSet & );
 
     virtual std::unique_ptr<SfxItemSet> Clone( bool bItems = true, SfxItemPool *pToPool = nullptr ) const override;
-    virtual SfxItemSet CloneAsValue( bool bItems = true, SfxItemPool *pToPool = nullptr ) const override;
 private:
     virtual const SfxPoolItem*  PutImpl( const SfxPoolItem&, sal_uInt16 nWhich, bool bPassingOwnership ) override;
 };
diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx
index 04fab3c9306e..bfaf0a60e169 100644
--- a/svl/source/items/itemset.cxx
+++ b/svl/source/items/itemset.cxx
@@ -1251,6 +1251,10 @@ std::unique_ptr<SfxItemSet> SfxItemSet::Clone(bool bItems, SfxItemPool *pToPool
 
 SfxItemSet SfxItemSet::CloneAsValue(bool bItems, SfxItemPool *pToPool ) const
 {
+    // if you are trying to clone, then the thing you are cloning is polymorphic, which means
+    // it cannot be cloned as a value
+    assert((typeid(*this) == typeid(SfxItemSet)) && "cannot call this on a subclass of SfxItemSet");
+
     if (pToPool && pToPool != m_pPool)
     {
         SfxItemSet aNewSet(*pToPool, m_pWhichRanges);
@@ -1386,15 +1390,6 @@ std::unique_ptr<SfxItemSet> SfxAllItemSet::Clone(bool bItems, SfxItemPool *pToPo
         return std::unique_ptr<SfxItemSet>(bItems ? new SfxAllItemSet(*this) : new SfxAllItemSet(*m_pPool));
 }
 
-SfxItemSet SfxAllItemSet::CloneAsValue(bool , SfxItemPool * ) const
-{
-    // if you are trying to clone, then the thing you are cloning is polymorphic, which means
-    // it cannot be cloned as a value
-    throw std::logic_error("cannot do this");
-}
-
-
-
 
 WhichRangesContainer::WhichRangesContainer( const WhichPair* wids, sal_Int32 nSize )
 {
diff --git a/sw/source/core/txtnode/thints.cxx b/sw/source/core/txtnode/thints.cxx
index 45e5e2f099f4..a6d72708c2f6 100644
--- a/sw/source/core/txtnode/thints.cxx
+++ b/sw/source/core/txtnode/thints.cxx
@@ -914,7 +914,7 @@ void SwpHints::BuildPortions( SwTextNode& rNode, SwTextAttr& rNewHint,
                 // #i81764# This should not be applied for no length attributes!!! <--
                 if ( !bNoLengthAttribute && rNode.HasSwAttrSet() && pNewStyle->Count() )
                 {
-                    std::optional<SfxItemSet> oNewSet;
+                    std::unique_ptr<SfxItemSet> pNewSet;
 
                     SfxItemIter aIter2( *pNewStyle );
                     const SfxPoolItem* pItem = aIter2.GetCurItem();
@@ -929,19 +929,19 @@ void SwpHints::BuildPortions( SwTextNode& rNode, SwTextAttr& rNewHint,
                             // Do not clear item if the attribute is set in a character format:
                             if ( !pCurrentCharFormat || nullptr == CharFormat::GetItem( *pCurrentCharFormat, pItem->Which() ) )
                             {
-                                if ( !oNewSet )
-                                    oNewSet.emplace(pNewStyle->CloneAsValue());
-                                oNewSet->ClearItem( pItem->Which() );
+                                if ( !pNewSet )
+                                    pNewSet = pNewStyle->Clone();
+                                pNewSet->ClearItem( pItem->Which() );
                             }
                         }
                     }
                     while ((pItem = aIter2.NextItem()));
 
-                    if ( oNewSet )
+                    if ( pNewSet )
                     {
                         bOptimizeAllowed = false;
-                        if ( oNewSet->Count() )
-                            pNewStyle = rNode.getIDocumentStyleAccess().getAutomaticStyle( *oNewSet, IStyleAccess::AUTO_STYLE_CHAR );
+                        if ( pNewSet->Count() )
+                            pNewStyle = rNode.getIDocumentStyleAccess().getAutomaticStyle( *pNewSet, IStyleAccess::AUTO_STYLE_CHAR );
                         else
                             pNewStyle.reset();
                     }
@@ -1039,9 +1039,9 @@ SwTextAttr* MakeTextAttr(
         // If the attribute is an auto-style which refers to a pool that is
         // different from rDoc's pool, we have to correct this:
         const std::shared_ptr<SfxItemSet> & pAutoStyle = static_cast<const SwFormatAutoFormat&>(rAttr).GetStyleHandle();
-        SfxItemSet aNewSet =
-                pAutoStyle->SfxItemSet::CloneAsValue( true, &rDoc.GetAttrPool() );
-        SwTextAttr* pNew = MakeTextAttr( rDoc, aNewSet, nStt, nEnd );
+        std::unique_ptr<const SfxItemSet> pNewSet(
+                pAutoStyle->SfxItemSet::Clone( true, &rDoc.GetAttrPool() ));
+        SwTextAttr* pNew = MakeTextAttr( rDoc, *pNewSet, nStt, nEnd );
         return pNew;
     }