sc/inc/patattr.hxx | 6 ++++++
sc/source/core/data/patattr.cxx | 21 ++++++++++++++++++---
2 files changed, 24 insertions(+), 3 deletions(-)
New commits:
commit e81400196cd9c24be32552a19851da4162d51c7a
Author: Luboš Luňák <l.lu...@collabora.com>
AuthorDate: Sun Feb 20 12:38:29 2022 +0100
Commit: Luboš Luňák <l.lu...@collabora.com>
CommitDate: Sun Feb 20 20:46:41 2022 +0100
fix ScPatternAttr lookup hashing (tdf#135215)
I made a mistake in 98d51bf8ce13bdac2d71f50f58d6d0ddb by using
SfxItemSet::Count(), which returns a number of existing items.
But EqualPatternSets() compares all items, even empty ones,
so I should have used TotalCount() (which is actually hardcoded
as a number in the code). Without this, the hash usually didn't
include all items, leading to matching hashes even though
the items clearly didn't match.
Change-Id: I1e25752d3ddca2b63c3c295a9a425965531cd4d3
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/130210
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.gran...@collabora.co.uk>
Reviewed-by: Luboš Luňák <l.lu...@collabora.com>
diff --git a/sc/inc/patattr.hxx b/sc/inc/patattr.hxx
index c449190b2305..261cccc8cf00 100644
--- a/sc/inc/patattr.hxx
+++ b/sc/inc/patattr.hxx
@@ -145,6 +145,12 @@ public:
void SetKey(sal_uInt64 nKey);
sal_uInt64 GetKey() const;
+ // TODO: tdf#135215: This is a band-aid to detect changes and invalidate
the hash,
+ // a proper way would be probably to override SfxItemSet::Changed(), but
6cb400f41df0dd10
+ // hardcoded SfxSetItem to contain SfxItemSet.
+ SfxItemSet& GetItemSet() { mxHashCode.reset(); return
SfxSetItem::GetItemSet(); }
+ using SfxSetItem::GetItemSet;
+
private:
void CalcHashCode() const;
};
diff --git a/sc/source/core/data/patattr.cxx b/sc/source/core/data/patattr.cxx
index 0414b957cada..aaab0183fa19 100644
--- a/sc/source/core/data/patattr.cxx
+++ b/sc/source/core/data/patattr.cxx
@@ -121,6 +121,8 @@ static bool StrCmp( const OUString* pStr1, const OUString*
pStr2 )
return *pStr1 == *pStr2;
}
+constexpr size_t compareSize = ATTR_PATTERN_END - ATTR_PATTERN_START + 1;
+
static bool EqualPatternSets( const SfxItemSet& rSet1, const SfxItemSet& rSet2
)
{
// #i62090# The SfxItemSet in the SfxSetItem base class always has the
same ranges
@@ -130,10 +132,15 @@ static bool EqualPatternSets( const SfxItemSet& rSet1,
const SfxItemSet& rSet2 )
if ( rSet1.Count() != rSet2.Count() )
return false;
+ // 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.
+ if( rSet1.TotalCount() != compareSize || rSet2.TotalCount() != compareSize
)
+ return rSet1 == rSet2;
+
SfxPoolItem const ** pItems1 = rSet1.GetItems_Impl(); // inline method
of SfxItemSet
SfxPoolItem const ** pItems2 = rSet2.GetItems_Impl();
- return ( 0 == memcmp( pItems1, pItems2, (ATTR_PATTERN_END -
ATTR_PATTERN_START + 1) * sizeof(pItems1[0]) ) );
+ return ( 0 == memcmp( pItems1, pItems2, compareSize * sizeof(pItems1[0]) )
);
}
bool ScPatternAttr::operator==( const SfxPoolItem& rCmp ) const
@@ -155,7 +162,7 @@ bool ScPatternAttr::operator==( const SfxPoolItem& rCmp )
const
size_t ScPatternAttr::LookupHashCode() const
{
- if (!mxHashCode)
+ if (SAL_UNLIKELY(!mxHashCode))
CalcHashCode();
return *mxHashCode;
}
@@ -1197,6 +1204,7 @@ void ScPatternAttr::SetStyleSheet( ScStyleSheet*
pNewStyle, bool bClearDirectFor
GetItemSet().SetParent(nullptr);
pStyle = nullptr;
}
+ mxHashCode.reset();
}
void ScPatternAttr::UpdateStyleSheet(const ScDocument& rDoc)
@@ -1222,6 +1230,7 @@ void ScPatternAttr::UpdateStyleSheet(const ScDocument&
rDoc)
}
else
pStyle = nullptr;
+ mxHashCode.reset();
}
void ScPatternAttr::StyleToName()
@@ -1233,6 +1242,7 @@ void ScPatternAttr::StyleToName()
pName = pStyle->GetName();
pStyle = nullptr;
GetItemSet().SetParent( nullptr );
+ mxHashCode.reset();
}
}
@@ -1373,8 +1383,13 @@ sal_uInt64 ScPatternAttr::GetKey() const
void ScPatternAttr::CalcHashCode() const
{
auto const & rSet = GetItemSet();
+ if( rSet.TotalCount() != compareSize ) // see EqualPatternSets()
+ {
+ mxHashCode = 0; // invalid
+ return;
+ }
mxHashCode = 1; // Set up seed so that an empty pattern does not have an
(invalid) hash of 0.
- boost::hash_range(*mxHashCode, rSet.GetItems_Impl(), rSet.GetItems_Impl()
+ rSet.Count());
+ boost::hash_range(*mxHashCode, rSet.GetItems_Impl(), rSet.GetItems_Impl()
+ compareSize);
}
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
