From 184b82553000a41fd0c90be56fa2f1c1503e2e9e Mon Sep 17 00:00:00 2001 From: tsepez Date: Mon, 11 Apr 2016 10:56:00 -0700 Subject: Avoid copying in TrimRight() and TrimLeft() if possible. Make Byte and Wide code identical while at it. Review URL: https://codereview.chromium.org/1877553002 --- core/fxcrt/fx_basic_bstring.cpp | 4 +-- core/fxcrt/fx_basic_bstring_unittest.cpp | 62 ++++++++++++++++++++++++++++++++ core/fxcrt/fx_basic_wstring.cpp | 23 ++++++------ core/fxcrt/fx_basic_wstring_unittest.cpp | 62 ++++++++++++++++++++++++++++++++ core/fxcrt/include/fx_string.h | 12 +++---- 5 files changed, 145 insertions(+), 18 deletions(-) diff --git a/core/fxcrt/fx_basic_bstring.cpp b/core/fxcrt/fx_basic_bstring.cpp index 5b9c683c53..4a48539578 100644 --- a/core/fxcrt/fx_basic_bstring.cpp +++ b/core/fxcrt/fx_basic_bstring.cpp @@ -841,7 +841,6 @@ void CFX_ByteString::TrimRight(const CFX_ByteStringC& pTargets) { if (!m_pData || pTargets.IsEmpty()) { return; } - ReallocBeforeWrite(m_pData->m_nDataLength); FX_STRSIZE pos = GetLength(); if (pos < 1) { return; @@ -858,6 +857,7 @@ void CFX_ByteString::TrimRight(const CFX_ByteStringC& pTargets) { 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 @@ void CFX_ByteString::TrimLeft(const CFX_ByteStringC& pTargets) { if (len < 1) return; - ReallocBeforeWrite(len); FX_STRSIZE pos = 0; while (pos < len) { FX_STRSIZE i = 0; @@ -892,6 +891,7 @@ void CFX_ByteString::TrimLeft(const CFX_ByteStringC& pTargets) { 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 73235ddf5f..8c72b2a054 100644 --- a/core/fxcrt/fx_basic_bstring_unittest.cpp +++ b/core/fxcrt/fx_basic_bstring_unittest.cpp @@ -505,6 +505,37 @@ TEST(fxcrt, ByteStringTrimRight) { 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 @@ TEST(fxcrt, ByteStringTrimLeft) { 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 b5ccaf6da5..08a9d212d4 100644 --- a/core/fxcrt/fx_basic_wstring.cpp +++ b/core/fxcrt/fx_basic_wstring.cpp @@ -840,7 +840,6 @@ void CFX_WideString::TrimRight(const CFX_WideStringC& pTargets) { if (!m_pData || pTargets.IsEmpty()) { return; } - ReallocBeforeWrite(m_pData->m_nDataLength); FX_STRSIZE pos = GetLength(); if (pos < 1) { return; @@ -852,6 +851,7 @@ void CFX_WideString::TrimRight(const CFX_WideStringC& pTargets) { 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 @@ void CFX_WideString::TrimLeft(const CFX_WideStringC& pTargets) { 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 cfc190f72a..bc38141a95 100644 --- a/core/fxcrt/fx_basic_wstring_unittest.cpp +++ b/core/fxcrt/fx_basic_wstring_unittest.cpp @@ -472,6 +472,37 @@ TEST(fxcrt, WideStringTrimRight) { 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 @@ TEST(fxcrt, WideStringTrimLeft) { 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 0e3160a9d8..ef0dfee6fb 100644 --- a/core/fxcrt/include/fx_string.h +++ b/core/fxcrt/include/fx_string.h @@ -167,21 +167,21 @@ class CFX_ByteString { 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(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 @@ class CFX_WideString { 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()); } -- cgit v1.2.3