Avoid copying in TrimRight() and TrimLeft() if possible.
Make Byte and Wide code identical while at it.
Review URL: https://codereview.chromium.org/1877553002
diff --git a/core/fxcrt/fx_basic_bstring.cpp b/core/fxcrt/fx_basic_bstring.cpp
index 5b9c683..4a48539 100644
--- a/core/fxcrt/fx_basic_bstring.cpp
+++ b/core/fxcrt/fx_basic_bstring.cpp
@@ -841,7 +841,6 @@
if (!m_pData || pTargets.IsEmpty()) {
return;
}
- ReallocBeforeWrite(m_pData->m_nDataLength);
FX_STRSIZE pos = GetLength();
if (pos < 1) {
return;
@@ -858,6 +857,7 @@
pos--;
}
if (pos < m_pData->m_nDataLength) {
+ ReallocBeforeWrite(m_pData->m_nDataLength);
m_pData->m_String[pos] = 0;
m_pData->m_nDataLength = pos;
}
@@ -879,7 +879,6 @@
if (len < 1)
return;
- ReallocBeforeWrite(len);
FX_STRSIZE pos = 0;
while (pos < len) {
FX_STRSIZE i = 0;
@@ -892,6 +891,7 @@
pos++;
}
if (pos) {
+ ReallocBeforeWrite(len);
FX_STRSIZE nDataLength = len - pos;
FXSYS_memmove(m_pData->m_String, m_pData->m_String + pos,
(nDataLength + 1) * sizeof(FX_CHAR));
diff --git a/core/fxcrt/fx_basic_bstring_unittest.cpp b/core/fxcrt/fx_basic_bstring_unittest.cpp
index 73235dd..8c72b2a 100644
--- a/core/fxcrt/fx_basic_bstring_unittest.cpp
+++ b/core/fxcrt/fx_basic_bstring_unittest.cpp
@@ -505,6 +505,37 @@
EXPECT_EQ("", empty);
}
+TEST(fxcrt, ByteStringTrimRightCopies) {
+ {
+ // With a single reference, no copy takes place.
+ CFX_ByteString fred(" FRED ");
+ const FX_CHAR* old_buffer = fred.c_str();
+ fred.TrimRight();
+ EXPECT_EQ(" FRED", fred);
+ EXPECT_EQ(old_buffer, fred.c_str());
+ }
+ {
+ // With multiple references, we must copy.
+ CFX_ByteString fred(" FRED ");
+ CFX_ByteString other_fred = fred;
+ const FX_CHAR* old_buffer = fred.c_str();
+ fred.TrimRight();
+ EXPECT_EQ(" FRED", fred);
+ EXPECT_EQ(" FRED ", other_fred);
+ EXPECT_NE(old_buffer, fred.c_str());
+ }
+ {
+ // With multiple references, but no modifications, no copy.
+ CFX_ByteString fred("FRED");
+ CFX_ByteString other_fred = fred;
+ const FX_CHAR* old_buffer = fred.c_str();
+ fred.TrimRight();
+ EXPECT_EQ("FRED", fred);
+ EXPECT_EQ("FRED", other_fred);
+ EXPECT_EQ(old_buffer, fred.c_str());
+ }
+}
+
TEST(fxcrt, ByteStringTrimLeft) {
CFX_ByteString fred(" FRED ");
fred.TrimLeft();
@@ -533,6 +564,37 @@
EXPECT_EQ("", empty);
}
+TEST(fxcrt, ByteStringTrimLeftCopies) {
+ {
+ // With a single reference, no copy takes place.
+ CFX_ByteString fred(" FRED ");
+ const FX_CHAR* old_buffer = fred.c_str();
+ fred.TrimLeft();
+ EXPECT_EQ("FRED ", fred);
+ EXPECT_EQ(old_buffer, fred.c_str());
+ }
+ {
+ // With multiple references, we must copy.
+ CFX_ByteString fred(" FRED ");
+ CFX_ByteString other_fred = fred;
+ const FX_CHAR* old_buffer = fred.c_str();
+ fred.TrimLeft();
+ EXPECT_EQ("FRED ", fred);
+ EXPECT_EQ(" FRED ", other_fred);
+ EXPECT_NE(old_buffer, fred.c_str());
+ }
+ {
+ // With multiple references, but no modifications, no copy.
+ CFX_ByteString fred("FRED");
+ CFX_ByteString other_fred = fred;
+ const FX_CHAR* old_buffer = fred.c_str();
+ fred.TrimLeft();
+ EXPECT_EQ("FRED", fred);
+ EXPECT_EQ("FRED", other_fred);
+ EXPECT_EQ(old_buffer, fred.c_str());
+ }
+}
+
TEST(fxcrt, ByteStringCNotNull) {
CFX_ByteStringC string3("abc");
CFX_ByteStringC string6("abcdef");
diff --git a/core/fxcrt/fx_basic_wstring.cpp b/core/fxcrt/fx_basic_wstring.cpp
index b5ccaf6..08a9d21 100644
--- a/core/fxcrt/fx_basic_wstring.cpp
+++ b/core/fxcrt/fx_basic_wstring.cpp
@@ -840,7 +840,6 @@
if (!m_pData || pTargets.IsEmpty()) {
return;
}
- ReallocBeforeWrite(m_pData->m_nDataLength);
FX_STRSIZE pos = GetLength();
if (pos < 1) {
return;
@@ -852,6 +851,7 @@
pos--;
}
if (pos < m_pData->m_nDataLength) {
+ ReallocBeforeWrite(m_pData->m_nDataLength);
m_pData->m_String[pos] = 0;
m_pData->m_nDataLength = pos;
}
@@ -874,18 +874,21 @@
if (len < 1)
return;
- ReallocBeforeWrite(len);
- const FX_WCHAR* lpsz = m_pData->m_String;
- while (*lpsz != 0) {
- if (!FXSYS_wcschr(pTargets.c_str(), *lpsz)) {
+ FX_STRSIZE pos = 0;
+ while (pos < len) {
+ FX_STRSIZE i = 0;
+ while (i < pTargets.GetLength() && pTargets[i] != m_pData->m_String[pos]) {
+ i++;
+ }
+ if (i == pTargets.GetLength()) {
break;
}
- lpsz++;
+ pos++;
}
- if (lpsz != m_pData->m_String) {
- int nDataLength =
- m_pData->m_nDataLength - (FX_STRSIZE)(lpsz - m_pData->m_String);
- FXSYS_memmove(m_pData->m_String, lpsz,
+ if (pos) {
+ ReallocBeforeWrite(len);
+ FX_STRSIZE nDataLength = len - pos;
+ FXSYS_memmove(m_pData->m_String, m_pData->m_String + pos,
(nDataLength + 1) * sizeof(FX_WCHAR));
m_pData->m_nDataLength = nDataLength;
}
diff --git a/core/fxcrt/fx_basic_wstring_unittest.cpp b/core/fxcrt/fx_basic_wstring_unittest.cpp
index cfc190f..bc38141 100644
--- a/core/fxcrt/fx_basic_wstring_unittest.cpp
+++ b/core/fxcrt/fx_basic_wstring_unittest.cpp
@@ -472,6 +472,37 @@
EXPECT_EQ(L"", empty);
}
+TEST(fxcrt, WideStringTrimRightCopies) {
+ {
+ // With a single reference, no copy takes place.
+ CFX_WideString fred(L" FRED ");
+ const FX_WCHAR* old_buffer = fred.c_str();
+ fred.TrimRight();
+ EXPECT_EQ(L" FRED", fred);
+ EXPECT_EQ(old_buffer, fred.c_str());
+ }
+ {
+ // With multiple references, we must copy.
+ CFX_WideString fred(L" FRED ");
+ CFX_WideString other_fred = fred;
+ const FX_WCHAR* old_buffer = fred.c_str();
+ fred.TrimRight();
+ EXPECT_EQ(L" FRED", fred);
+ EXPECT_EQ(L" FRED ", other_fred);
+ EXPECT_NE(old_buffer, fred.c_str());
+ }
+ {
+ // With multiple references, but no modifications, no copy.
+ CFX_WideString fred(L"FRED");
+ CFX_WideString other_fred = fred;
+ const FX_WCHAR* old_buffer = fred.c_str();
+ fred.TrimRight();
+ EXPECT_EQ(L"FRED", fred);
+ EXPECT_EQ(L"FRED", other_fred);
+ EXPECT_EQ(old_buffer, fred.c_str());
+ }
+}
+
TEST(fxcrt, WideStringTrimLeft) {
CFX_WideString fred(L" FRED ");
fred.TrimLeft();
@@ -500,6 +531,37 @@
EXPECT_EQ(L"", empty);
}
+TEST(fxcrt, WideStringTrimLeftCopies) {
+ {
+ // With a single reference, no copy takes place.
+ CFX_WideString fred(L" FRED ");
+ const FX_WCHAR* old_buffer = fred.c_str();
+ fred.TrimLeft();
+ EXPECT_EQ(L"FRED ", fred);
+ EXPECT_EQ(old_buffer, fred.c_str());
+ }
+ {
+ // With multiple references, we must copy.
+ CFX_WideString fred(L" FRED ");
+ CFX_WideString other_fred = fred;
+ const FX_WCHAR* old_buffer = fred.c_str();
+ fred.TrimLeft();
+ EXPECT_EQ(L"FRED ", fred);
+ EXPECT_EQ(L" FRED ", other_fred);
+ EXPECT_NE(old_buffer, fred.c_str());
+ }
+ {
+ // With multiple references, but no modifications, no copy.
+ CFX_WideString fred(L"FRED");
+ CFX_WideString other_fred = fred;
+ const FX_WCHAR* old_buffer = fred.c_str();
+ fred.TrimLeft();
+ EXPECT_EQ(L"FRED", fred);
+ EXPECT_EQ(L"FRED", other_fred);
+ EXPECT_EQ(old_buffer, fred.c_str());
+ }
+}
+
TEST(fxcrt, WideStringUTF16LE_Encode) {
struct UTF16LEEncodeCase {
CFX_WideString ws;
diff --git a/core/fxcrt/include/fx_string.h b/core/fxcrt/include/fx_string.h
index 0e3160a..ef0dfee 100644
--- a/core/fxcrt/include/fx_string.h
+++ b/core/fxcrt/include/fx_string.h
@@ -167,21 +167,21 @@
static CFX_ByteString FromUnicode(const CFX_WideString& str);
// Explicit conversion to C-style string.
- // Note: |this| must outlive the use of the result.
+ // Note: Any subsequent modification of |this| will invalidate the result.
const FX_CHAR* c_str() const { return m_pData ? m_pData->m_String : ""; }
// Implicit conversion to C-style string -- deprecated.
operator const FX_CHAR*() const { return m_pData ? m_pData->m_String : ""; }
// Explicit conversion to uint8_t*.
- // Note: |this| must outlive the use of the result.
+ // Note: Any subsequent modification of |this| will invalidate the result.
const uint8_t* raw_str() const {
return m_pData ? reinterpret_cast<const uint8_t*>(m_pData->m_String)
: nullptr;
}
// Explicit conversion to CFX_ByteStringC.
- // Note: |this| must outlive the use of the result.
+ // Note: Any subsequent modification of |this| will invalidate the result.
CFX_ByteStringC AsStringC() const {
return CFX_ByteStringC(raw_str(), GetLength());
}
@@ -497,15 +497,15 @@
static FX_STRSIZE WStringLength(const unsigned short* str);
// Explicit conversion to C-style wide string.
- // Note: |this| must outlive the use of the result.
+ // Note: Any subsequent modification of |this| will invalidate the result.
const FX_WCHAR* c_str() const { return m_pData ? m_pData->m_String : L""; }
// Implicit conversion to C-style wide string -- deprecated.
- // Note: |this| must outlive the use of the result.
+ // Note: Any subsequent modification of |this| will invalidate the result.
operator const FX_WCHAR*() const { return m_pData ? m_pData->m_String : L""; }
// Explicit conversion to CFX_WideStringC.
- // Note: |this| must outlive the use of the result.
+ // Note: Any subsequent modification of |this| will invalidate the result.
CFX_WideStringC AsStringC() const {
return CFX_WideStringC(c_str(), GetLength());
}
