commit e09c1e4db92e28a332f55aa3c80ceb44f4b74287
author tsepez <tsepez@chromium.org> Mon Apr 11 18:01:13 2016 -0700
committer Commit bot <commit-bot@chromium.org> Mon Apr 11 18:01:13 2016 -0700
tree 2ba0b13dadf379f7673317fdd2909db21cce9412
parent 24a48881c5407651a58cfd6547ac9b6a9823a63e

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

diff --git a/core/fxcrt/fx_basic_bstring.cpp b/core/fxcrt/fx_basic_bstring.cpp
index 4a48539..23fdcaa 100644
--- a/core/fxcrt/fx_basic_bstring.cpp
+++ b/core/fxcrt/fx_basic_bstring.cpp
@@ -719,17 +719,30 @@
   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;
       pstrDest++;
     }
     pstrSource++;
   }
+
   *pstrDest = 0;
   FX_STRSIZE nCount = (FX_STRSIZE)(pstrSource - pstrDest);
   m_pData->m_nDataLength -= nCount;

core/fxcrt/fx_basic_bstring_unittest.cpp

diff --git a/core/fxcrt/fx_basic_bstring_unittest.cpp b/core/fxcrt/fx_basic_bstring_unittest.cpp
index 8c72b2a..ea7e17f 100644
--- a/core/fxcrt/fx_basic_bstring_unittest.cpp
+++ b/core/fxcrt/fx_basic_bstring_unittest.cpp
@@ -333,6 +333,35 @@
   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");

core/fxcrt/fx_basic_wstring.cpp

diff --git a/core/fxcrt/fx_basic_wstring.cpp b/core/fxcrt/fx_basic_wstring.cpp
index 08a9d21..069b9fa 100644
--- a/core/fxcrt/fx_basic_wstring.cpp
+++ b/core/fxcrt/fx_basic_wstring.cpp
@@ -672,17 +672,30 @@
   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;
       pstrDest++;
     }
     pstrSource++;
   }
+
   *pstrDest = 0;
   FX_STRSIZE nCount = (FX_STRSIZE)(pstrSource - pstrDest);
   m_pData->m_nDataLength -= nCount;

core/fxcrt/fx_basic_wstring_unittest.cpp

diff --git a/core/fxcrt/fx_basic_wstring_unittest.cpp b/core/fxcrt/fx_basic_wstring_unittest.cpp
index bc38141..708556a 100644
--- a/core/fxcrt/fx_basic_wstring_unittest.cpp
+++ b/core/fxcrt/fx_basic_wstring_unittest.cpp
@@ -294,6 +294,35 @@
   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");