From e09c1e4db92e28a332f55aa3c80ceb44f4b74287 Mon Sep 17 00:00:00 2001 From: tsepez Date: Mon, 11 Apr 2016 18:01:13 -0700 Subject: Make CFX_{Byte,Wide}String::Remove() no-touch if possible Don't try to copy the string until we are sure we need to change it. Review URL: https://codereview.chromium.org/1877993002 --- core/fxcrt/fx_basic_bstring.cpp | 17 +++++++++++++++-- core/fxcrt/fx_basic_bstring_unittest.cpp | 29 +++++++++++++++++++++++++++++ core/fxcrt/fx_basic_wstring.cpp | 17 +++++++++++++++-- core/fxcrt/fx_basic_wstring_unittest.cpp | 29 +++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 4 deletions(-) (limited to 'core') diff --git a/core/fxcrt/fx_basic_bstring.cpp b/core/fxcrt/fx_basic_bstring.cpp index 4a48539578..23fdcaa25e 100644 --- a/core/fxcrt/fx_basic_bstring.cpp +++ b/core/fxcrt/fx_basic_bstring.cpp @@ -719,10 +719,22 @@ FX_STRSIZE CFX_ByteString::Remove(FX_CHAR chRemove) { if (!m_pData || m_pData->m_nDataLength < 1) return 0; - ReallocBeforeWrite(m_pData->m_nDataLength); FX_CHAR* pstrSource = m_pData->m_String; - FX_CHAR* pstrDest = m_pData->m_String; FX_CHAR* pstrEnd = m_pData->m_String + m_pData->m_nDataLength; + while (pstrSource < pstrEnd) { + if (*pstrSource == chRemove) + break; + pstrSource++; + } + if (pstrSource == pstrEnd) + return 0; + + ptrdiff_t copied = pstrSource - m_pData->m_String; + ReallocBeforeWrite(m_pData->m_nDataLength); + pstrSource = m_pData->m_String + copied; + pstrEnd = m_pData->m_String + m_pData->m_nDataLength; + + FX_CHAR* pstrDest = pstrSource; while (pstrSource < pstrEnd) { if (*pstrSource != chRemove) { *pstrDest = *pstrSource; @@ -730,6 +742,7 @@ FX_STRSIZE CFX_ByteString::Remove(FX_CHAR chRemove) { } pstrSource++; } + *pstrDest = 0; FX_STRSIZE nCount = (FX_STRSIZE)(pstrSource - pstrDest); m_pData->m_nDataLength -= nCount; diff --git a/core/fxcrt/fx_basic_bstring_unittest.cpp b/core/fxcrt/fx_basic_bstring_unittest.cpp index 8c72b2a054..ea7e17f1f1 100644 --- a/core/fxcrt/fx_basic_bstring_unittest.cpp +++ b/core/fxcrt/fx_basic_bstring_unittest.cpp @@ -333,6 +333,35 @@ TEST(fxcrt, ByteStringRemove) { EXPECT_EQ("", empty); } +TEST(fxcrt, ByteStringRemoveCopies) { + CFX_ByteString freed("FREED"); + const FX_CHAR* old_buffer = freed.c_str(); + + // No change with single reference - no copy. + freed.Remove('Q'); + EXPECT_EQ("FREED", freed); + EXPECT_EQ(old_buffer, freed.c_str()); + + // Change with single reference - no copy. + freed.Remove('E'); + EXPECT_EQ("FRD", freed); + EXPECT_EQ(old_buffer, freed.c_str()); + + // No change with multiple references - no copy. + CFX_ByteString shared(freed); + freed.Remove('Q'); + EXPECT_EQ("FRD", freed); + EXPECT_EQ(old_buffer, freed.c_str()); + EXPECT_EQ(old_buffer, shared.c_str()); + + // Change with multiple references -- must copy. + freed.Remove('D'); + EXPECT_EQ("FR", freed); + EXPECT_NE(old_buffer, freed.c_str()); + EXPECT_EQ("FRD", shared); + EXPECT_EQ(old_buffer, shared.c_str()); +} + TEST(fxcrt, ByteStringReplace) { CFX_ByteString fred("FRED"); fred.Replace("FR", "BL"); diff --git a/core/fxcrt/fx_basic_wstring.cpp b/core/fxcrt/fx_basic_wstring.cpp index 08a9d212d4..069b9fa42d 100644 --- a/core/fxcrt/fx_basic_wstring.cpp +++ b/core/fxcrt/fx_basic_wstring.cpp @@ -672,10 +672,22 @@ FX_STRSIZE CFX_WideString::Remove(FX_WCHAR chRemove) { if (!m_pData || m_pData->m_nDataLength < 1) return 0; - ReallocBeforeWrite(m_pData->m_nDataLength); FX_WCHAR* pstrSource = m_pData->m_String; - FX_WCHAR* pstrDest = m_pData->m_String; FX_WCHAR* pstrEnd = m_pData->m_String + m_pData->m_nDataLength; + while (pstrSource < pstrEnd) { + if (*pstrSource == chRemove) + break; + pstrSource++; + } + if (pstrSource == pstrEnd) + return 0; + + ptrdiff_t copied = pstrSource - m_pData->m_String; + ReallocBeforeWrite(m_pData->m_nDataLength); + pstrSource = m_pData->m_String + copied; + pstrEnd = m_pData->m_String + m_pData->m_nDataLength; + + FX_WCHAR* pstrDest = pstrSource; while (pstrSource < pstrEnd) { if (*pstrSource != chRemove) { *pstrDest = *pstrSource; @@ -683,6 +695,7 @@ FX_STRSIZE CFX_WideString::Remove(FX_WCHAR chRemove) { } pstrSource++; } + *pstrDest = 0; FX_STRSIZE nCount = (FX_STRSIZE)(pstrSource - pstrDest); m_pData->m_nDataLength -= nCount; diff --git a/core/fxcrt/fx_basic_wstring_unittest.cpp b/core/fxcrt/fx_basic_wstring_unittest.cpp index bc38141a95..708556af1d 100644 --- a/core/fxcrt/fx_basic_wstring_unittest.cpp +++ b/core/fxcrt/fx_basic_wstring_unittest.cpp @@ -294,6 +294,35 @@ TEST(fxcrt, WideStringRemove) { EXPECT_EQ(L"", empty); } +TEST(fxcrt, WideStringRemoveCopies) { + CFX_WideString freed(L"FREED"); + const FX_WCHAR* old_buffer = freed.c_str(); + + // No change with single reference - no copy. + freed.Remove(L'Q'); + EXPECT_EQ(L"FREED", freed); + EXPECT_EQ(old_buffer, freed.c_str()); + + // Change with single reference - no copy. + freed.Remove(L'E'); + EXPECT_EQ(L"FRD", freed); + EXPECT_EQ(old_buffer, freed.c_str()); + + // No change with multiple references - no copy. + CFX_WideString shared(freed); + freed.Remove(L'Q'); + EXPECT_EQ(L"FRD", freed); + EXPECT_EQ(old_buffer, freed.c_str()); + EXPECT_EQ(old_buffer, shared.c_str()); + + // Change with multiple references -- must copy. + freed.Remove(L'D'); + EXPECT_EQ(L"FR", freed); + EXPECT_NE(old_buffer, freed.c_str()); + EXPECT_EQ(L"FRD", shared); + EXPECT_EQ(old_buffer, shared.c_str()); +} + TEST(fxcrt, WideStringReplace) { CFX_WideString fred(L"FRED"); fred.Replace(L"FR", L"BL"); -- cgit v1.2.3