include/svl/sharedstringpool.hxx | 3 + sc/qa/unit/ucalc.cxx | 13 ++++-- svl/qa/unit/svl.cxx | 67 +++++++++++++++++++++++++---------- svl/source/misc/sharedstringpool.cxx | 3 + 4 files changed, 61 insertions(+), 25 deletions(-)
New commits: commit e47e0cb0ad1dc3554e9b57f8562a217cf785edbf Author: Luboš Luňák <l.lu...@collabora.com> AuthorDate: Thu Sep 22 10:34:04 2022 +0200 Commit: Eike Rathke <er...@redhat.com> CommitDate: Fri Sep 23 11:51:24 2022 +0200 make sure SharedString::EMPTY_STRING is interned in pools (tdf#150647) Without this, it may not actually be there, so interning "" would use a different string instance, and then comparing with SharedString::getEmptyString() would actually compare non-equal. Change-Id: I22660f63aa321e3a8f72cfb96df1db56e08fbb84 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/140402 Tested-by: Jenkins Reviewed-by: Eike Rathke <er...@redhat.com> diff --git a/include/svl/sharedstringpool.hxx b/include/svl/sharedstringpool.hxx index ff270eef5aa6..6880fec2a101 100644 --- a/include/svl/sharedstringpool.hxx +++ b/include/svl/sharedstringpool.hxx @@ -53,8 +53,9 @@ public: */ void purge(); + // For unit tests. Note that an "empty" pool may contain some internal items, + // such as SharedString::getEmptyString(). size_t getCount() const; - size_t getCountIgnoreCase() const; }; } diff --git a/sc/qa/unit/ucalc.cxx b/sc/qa/unit/ucalc.cxx index a63519f41224..d821a146eef8 100644 --- a/sc/qa/unit/ucalc.cxx +++ b/sc/qa/unit/ucalc.cxx @@ -383,6 +383,10 @@ void Test::testSharedStringPool() { m_pDoc->InsertTab(0, "foo"); + svl::SharedStringPool& rPool = m_pDoc->GetSharedStringPool(); + size_t extraCount = rPool.getCount(); // internal items such as SharedString::getEmptyString() + size_t extraCountIgnoreCase = rPool.getCountIgnoreCase(); + // Strings that are identical. m_pDoc->SetString(ScAddress(0,0,0), "Andy"); // A1 m_pDoc->SetString(ScAddress(0,1,0), "Andy"); // A2 @@ -420,10 +424,9 @@ void Test::testSharedStringPool() } // Check the string counts after purging. Purging shouldn't remove any strings in this case. - svl::SharedStringPool& rPool = m_pDoc->GetSharedStringPool(); rPool.purge(); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(5), rPool.getCount()); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), rPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL(5+extraCount, rPool.getCount()); + CPPUNIT_ASSERT_EQUAL(2+extraCountIgnoreCase, rPool.getCountIgnoreCase()); // Clear A1 clearRange(m_pDoc, ScAddress(0,0,0)); @@ -436,8 +439,8 @@ void Test::testSharedStringPool() // Clear A5 and the pool should be completely empty. clearRange(m_pDoc, ScAddress(0,4,0)); rPool.purge(); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), rPool.getCount()); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), rPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL(extraCount, rPool.getCount()); + CPPUNIT_ASSERT_EQUAL(extraCountIgnoreCase, rPool.getCountIgnoreCase()); // Now, compare string and edit text cells. m_pDoc->SetString(ScAddress(0,0,0), "Andy and Bruce"); // A1 diff --git a/svl/qa/unit/svl.cxx b/svl/qa/unit/svl.cxx index 7476ac339bdd..88add0a71793 100644 --- a/svl/qa/unit/svl.cxx +++ b/svl/qa/unit/svl.cxx @@ -48,6 +48,15 @@ static std::ostream& operator<<(std::ostream& rStrm, const Color& rColor) return rStrm; } +namespace svl +{ +static std::ostream& operator<<(std::ostream& rStrm, const SharedString& string ) +{ + return rStrm << "(" << static_cast<const void*>(string.getData()) << ")" << string.getString(); +} +} + + namespace { class Test : public CppUnit::TestFixture { @@ -62,6 +71,7 @@ public: void testSharedStringPool(); void testSharedStringPoolPurge(); void testSharedStringPoolPurgeBug1(); + void testSharedStringPoolEmptyString(); void testFdo60915(); void testI116701(); void testTdf103060(); @@ -80,6 +90,7 @@ public: CPPUNIT_TEST(testSharedStringPool); CPPUNIT_TEST(testSharedStringPoolPurge); CPPUNIT_TEST(testSharedStringPoolPurgeBug1); + CPPUNIT_TEST(testSharedStringPoolEmptyString); CPPUNIT_TEST(testFdo60915); CPPUNIT_TEST(testI116701); CPPUNIT_TEST(testTdf103060); @@ -363,18 +374,21 @@ void Test::testSharedStringPoolPurge() { SvtSysLocale aSysLocale; svl::SharedStringPool aPool(aSysLocale.GetCharClass()); + size_t extraCount = aPool.getCount(); // internal items such as SharedString::getEmptyString() + size_t extraCountIgnoreCase = aPool.getCountIgnoreCase(); + aPool.intern("Andy"); aPool.intern("andy"); aPool.intern("ANDY"); - CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong string count.", static_cast<size_t>(3), aPool.getCount()); - CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong case insensitive string count.", static_cast<size_t>(1), aPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong string count.", 3+extraCount, aPool.getCount()); + CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong case insensitive string count.", 1+extraCountIgnoreCase, aPool.getCountIgnoreCase()); // Since no string objects referencing the pooled strings exist, purging - // the pool should empty it. + // the pool should empty it (except for internal items). aPool.purge(); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), aPool.getCount()); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), aPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL(extraCount, aPool.getCount()); + CPPUNIT_ASSERT_EQUAL(extraCountIgnoreCase, aPool.getCountIgnoreCase()); // Now, create string objects using optional so we can clear them std::optional<svl::SharedString> pStr1 = aPool.intern("Andy"); @@ -382,37 +396,37 @@ void Test::testSharedStringPoolPurge() std::optional<svl::SharedString> pStr3 = aPool.intern("ANDY"); std::optional<svl::SharedString> pStr4 = aPool.intern("Bruce"); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(5), aPool.getCount()); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), aPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL(5+extraCount, aPool.getCount()); + CPPUNIT_ASSERT_EQUAL(2+extraCountIgnoreCase, aPool.getCountIgnoreCase()); // This shouldn't purge anything. aPool.purge(); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(5), aPool.getCount()); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), aPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL(5+extraCount, aPool.getCount()); + CPPUNIT_ASSERT_EQUAL(2+extraCountIgnoreCase, aPool.getCountIgnoreCase()); // Delete one heap string object, and purge. That should purge one string. pStr1.reset(); aPool.purge(); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(4), aPool.getCount()); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), aPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL(4+extraCount, aPool.getCount()); + CPPUNIT_ASSERT_EQUAL(2+extraCountIgnoreCase, aPool.getCountIgnoreCase()); // Nothing changes, because the upper-string is still in the map pStr3.reset(); aPool.purge(); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(4), aPool.getCount()); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), aPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL(4+extraCount, aPool.getCount()); + CPPUNIT_ASSERT_EQUAL(2+extraCountIgnoreCase, aPool.getCountIgnoreCase()); // Again. pStr2.reset(); aPool.purge(); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), aPool.getCount()); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), aPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL(2+extraCount, aPool.getCount()); + CPPUNIT_ASSERT_EQUAL(1+extraCountIgnoreCase, aPool.getCountIgnoreCase()); // Delete 'Bruce' and purge. pStr4.reset(); aPool.purge(); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), aPool.getCount()); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), aPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL(extraCount, aPool.getCount()); + CPPUNIT_ASSERT_EQUAL(extraCountIgnoreCase, aPool.getCountIgnoreCase()); } void Test::testSharedStringPoolPurgeBug1() @@ -421,11 +435,26 @@ void Test::testSharedStringPoolPurgeBug1() // purge() would de-reference a dangling pointer and consequently cause an ASAN failure. SvtSysLocale aSysLocale; svl::SharedStringPool aPool(aSysLocale.GetCharClass()); + size_t extraCount = aPool.getCount(); // internal items such as SharedString::getEmptyString() + size_t extraCountIgnoreCase = aPool.getCountIgnoreCase(); aPool.intern("Andy"); aPool.intern("andy"); aPool.purge(); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), aPool.getCount()); - CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), aPool.getCountIgnoreCase()); + CPPUNIT_ASSERT_EQUAL(extraCount, aPool.getCount()); + CPPUNIT_ASSERT_EQUAL(extraCountIgnoreCase, aPool.getCountIgnoreCase()); +} + +void Test::testSharedStringPoolEmptyString() +{ + // Make sure SharedString::getEmptyString() is in the pool and matches empty strings. + SvtSysLocale aSysLocale; + svl::SharedStringPool aPool(aSysLocale.GetCharClass()); + aPool.intern(""); + CPPUNIT_ASSERT_EQUAL(SharedString::getEmptyString(), aPool.intern("")); + CPPUNIT_ASSERT_EQUAL(SharedString::getEmptyString(), aPool.intern(SharedString::EMPTY_STRING)); + // And it should still work even after purging. + aPool.purge(); + CPPUNIT_ASSERT_EQUAL(SharedString::getEmptyString(), aPool.intern(SharedString::EMPTY_STRING)); } void Test::checkPreviewString(SvNumberFormatter& aFormatter, diff --git a/svl/source/misc/sharedstringpool.cxx b/svl/source/misc/sharedstringpool.cxx index 2fe8fd8a13ff..06cf8a5cef31 100644 --- a/svl/source/misc/sharedstringpool.cxx +++ b/svl/source/misc/sharedstringpool.cxx @@ -70,6 +70,9 @@ struct SharedStringPool::Impl SharedStringPool::SharedStringPool(const CharClass& rCharClass) : mpImpl(new Impl(rCharClass)) { + // make sure the one empty string instance is shared in this pool as well + intern(SharedString::EMPTY_STRING); + assert(intern(SharedString::EMPTY_STRING) == SharedString::getEmptyString()); } SharedStringPool::~SharedStringPool() {}