From 8a1758bf11c2d741e0cddc761b1dd2cdf564db93 Mon Sep 17 00:00:00 2001 From: Ryan Harrison Date: Tue, 15 Aug 2017 10:37:59 -0400 Subject: Remove GetAt from string classes This method duplicates the behaviour of the const [] operator and doesn't offer any additional safety. Folding them into one implementation. SetAt is retained, since implementing the non-const [] operator to replace SetAt has potential performance concerns. Specifically many non-obvious cases of reading an element using [] will cause a realloc & copy. BUG=pdfium:860 Change-Id: I3ef5e5e5a15376f040256b646eb0d90636e24b67 Reviewed-on: https://pdfium-review.googlesource.com/10870 Commit-Queue: Ryan Harrison Reviewed-by: Tom Sepez --- core/fxcrt/cfx_bytestring.cpp | 16 +++---- core/fxcrt/cfx_bytestring.h | 5 +-- core/fxcrt/cfx_bytestring_unittest.cpp | 79 ++++++++++++---------------------- core/fxcrt/cfx_chariter.cpp | 2 +- core/fxcrt/cfx_string_c_template.h | 6 +-- core/fxcrt/cfx_widestring.cpp | 12 +++--- core/fxcrt/cfx_widestring.h | 4 +- core/fxcrt/cfx_widestring_unittest.cpp | 77 ++++++++++++--------------------- core/fxcrt/xml/cxml_parser.cpp | 2 +- 9 files changed, 76 insertions(+), 127 deletions(-) (limited to 'core/fxcrt') diff --git a/core/fxcrt/cfx_bytestring.cpp b/core/fxcrt/cfx_bytestring.cpp index 0aba3be12a..cd049dcc5d 100644 --- a/core/fxcrt/cfx_bytestring.cpp +++ b/core/fxcrt/cfx_bytestring.cpp @@ -518,6 +518,12 @@ void CFX_ByteString::Format(const char* pFormat, ...) { va_end(argList); } +void CFX_ByteString::SetAt(FX_STRSIZE index, char c) { + ASSERT(index >= 0 && index < GetLength()); + ReallocBeforeWrite(m_pData->m_nDataLength); + m_pData->m_String[index] = c; +} + FX_STRSIZE CFX_ByteString::Insert(FX_STRSIZE index, char ch) { const FX_STRSIZE cur_length = m_pData ? m_pData->m_nDataLength : 0; if (index != pdfium::clamp(index, 0, cur_length)) @@ -694,12 +700,6 @@ FX_STRSIZE CFX_ByteString::Replace(const CFX_ByteStringC& pOld, return nCount; } -void CFX_ByteString::SetAt(FX_STRSIZE index, char c) { - ASSERT(index >= 0 && index < GetLength()); - ReallocBeforeWrite(m_pData->m_nDataLength); - m_pData->m_String[index] = c; -} - CFX_WideString CFX_ByteString::UTF8Decode() const { CFX_UTF8Decoder decoder; for (FX_STRSIZE i = 0; i < GetLength(); i++) { @@ -721,10 +721,10 @@ int CFX_ByteString::Compare(const CFX_ByteStringC& str) const { int that_len = str.GetLength(); int min_len = this_len < that_len ? this_len : that_len; for (int i = 0; i < min_len; i++) { - if ((uint8_t)m_pData->m_String[i] < str.GetAt(i)) { + if ((uint8_t)m_pData->m_String[i] < str[i]) { return -1; } - if ((uint8_t)m_pData->m_String[i] > str.GetAt(i)) { + if ((uint8_t)m_pData->m_String[i] > str[i]) { return 1; } } diff --git a/core/fxcrt/cfx_bytestring.h b/core/fxcrt/cfx_bytestring.h index df31751273..3e8b6619c2 100644 --- a/core/fxcrt/cfx_bytestring.h +++ b/core/fxcrt/cfx_bytestring.h @@ -107,14 +107,13 @@ class CFX_ByteString { const CFX_ByteString& operator+=(const CFX_ByteString& str); const CFX_ByteString& operator+=(const CFX_ByteStringC& bstrc); - uint8_t GetAt(FX_STRSIZE index) const { + const CharType& operator[](const FX_STRSIZE index) const { ASSERT(index >= 0 && index < GetLength()); return m_pData->m_String[index]; } - uint8_t operator[](FX_STRSIZE index) const { return GetAt(index); } - void SetAt(FX_STRSIZE index, char c); + FX_STRSIZE Insert(FX_STRSIZE index, char ch); FX_STRSIZE InsertAtFront(char ch) { return Insert(0, ch); } FX_STRSIZE InsertAtBack(char ch) { return Insert(GetLength(), ch); } diff --git a/core/fxcrt/cfx_bytestring_unittest.cpp b/core/fxcrt/cfx_bytestring_unittest.cpp index 680a37e342..35f407f6ec 100644 --- a/core/fxcrt/cfx_bytestring_unittest.cpp +++ b/core/fxcrt/cfx_bytestring_unittest.cpp @@ -11,26 +11,8 @@ #include "testing/gtest/include/gtest/gtest.h" #include "third_party/base/stl_util.h" -TEST(fxcrt, ByteStringGetAt) { - CFX_ByteString short_string("a"); - CFX_ByteString longer_string("abc"); - CFX_ByteString embedded_nul_string("ab\0c", 4); - -#ifndef NDEBUG - EXPECT_DEATH({ short_string.GetAt(-1); }, ".*"); -#endif - EXPECT_EQ('a', short_string.GetAt(0)); -#ifndef NDEBUG - EXPECT_DEATH({ short_string.GetAt(1); }, ".*"); -#endif - EXPECT_EQ('c', longer_string.GetAt(2)); - EXPECT_EQ('b', embedded_nul_string.GetAt(1)); - EXPECT_EQ('\0', embedded_nul_string.GetAt(2)); - EXPECT_EQ('c', embedded_nul_string.GetAt(3)); -} - -TEST(fxcrt, ByteStringOperatorSubscript) { - CFX_ByteString abc("abc"); +TEST(fxcrt, ByteStringElementAccess) { + const CFX_ByteString abc("abc"); #ifndef NDEBUG EXPECT_DEATH({ abc[-1]; }, ".*"); #endif @@ -40,22 +22,35 @@ TEST(fxcrt, ByteStringOperatorSubscript) { #ifndef NDEBUG EXPECT_DEATH({ abc[3]; }, ".*"); #endif -} -TEST(fxcrt, ByteStringSetAt) { - // CFX_ByteString includes the NUL terminator for non-empty strings. - CFX_ByteString abc("abc"); + CFX_ByteString mutable_abc = abc; + EXPECT_EQ(abc.c_str(), mutable_abc.c_str()); + EXPECT_EQ('a', mutable_abc[0]); + EXPECT_EQ('b', mutable_abc[1]); + EXPECT_EQ('c', mutable_abc[2]); + EXPECT_EQ(abc.c_str(), mutable_abc.c_str()); + #ifndef NDEBUG - EXPECT_DEATH({ abc.SetAt(-1, 'd'); }, ".*"); + EXPECT_DEATH({ mutable_abc.SetAt(-1, 'd'); }, ".*"); + EXPECT_EQ("abc", abc); #endif - abc.SetAt(0, 'd'); - EXPECT_EQ("dbc", abc); - abc.SetAt(1, 'e'); - EXPECT_EQ("dec", abc); - abc.SetAt(2, 'f'); - EXPECT_EQ("def", abc); + const char* c_str = abc.c_str(); + mutable_abc.SetAt(0, 'd'); + EXPECT_EQ(c_str, abc.c_str()); + EXPECT_NE(c_str, mutable_abc.c_str()); + EXPECT_EQ("abc", abc); + EXPECT_EQ("dbc", mutable_abc); + + mutable_abc.SetAt(1, 'e'); + EXPECT_EQ("abc", abc); + EXPECT_EQ("dec", mutable_abc); + + mutable_abc.SetAt(2, 'f'); + EXPECT_EQ("abc", abc); + EXPECT_EQ("def", mutable_abc); #ifndef NDEBUG - EXPECT_DEATH({ abc.SetAt(3, 'g'); }, ".*"); + EXPECT_DEATH({ mutable_abc.SetAt(3, 'g'); }, ".*"); + EXPECT_EQ("abc", abc); #endif } @@ -943,25 +938,7 @@ TEST(fxcrt, ByteStringCMid) { EXPECT_EQ(trailing_substring, longer_string.Mid(4, 3)); } -TEST(fxcrt, ByteStringCGetAt) { - CFX_ByteStringC short_string("a"); - CFX_ByteStringC longer_string("abc"); - CFX_ByteStringC embedded_nul_string("ab\0c", 4); - -#ifndef NDEBUG - EXPECT_DEATH({ short_string.GetAt(-1); }, ".*"); -#endif - EXPECT_EQ('a', static_cast(short_string.GetAt(0))); -#ifndef NDEBUG - EXPECT_DEATH({ short_string.GetAt(1); }, ".*"); -#endif - EXPECT_EQ('c', static_cast(longer_string.GetAt(2))); - EXPECT_EQ('b', static_cast(embedded_nul_string.GetAt(1))); - EXPECT_EQ('\0', static_cast(embedded_nul_string.GetAt(2))); - EXPECT_EQ('c', static_cast(embedded_nul_string.GetAt(3))); -} - -TEST(fxcrt, ByteStringCOperatorSubscript) { +TEST(fxcrt, ByteStringCElementAccess) { // CFX_ByteStringC includes the NUL terminator for non-empty strings. CFX_ByteStringC abc("abc"); #ifndef NDEBUG diff --git a/core/fxcrt/cfx_chariter.cpp b/core/fxcrt/cfx_chariter.cpp index b726a7e484..d26dd2df8f 100644 --- a/core/fxcrt/cfx_chariter.cpp +++ b/core/fxcrt/cfx_chariter.cpp @@ -29,7 +29,7 @@ bool CFX_CharIter::Next(bool bPrev) { } wchar_t CFX_CharIter::GetChar() { - return m_wsText.GetAt(m_nIndex); + return m_wsText[m_nIndex]; } void CFX_CharIter::SetAt(int32_t nIndex) { diff --git a/core/fxcrt/cfx_string_c_template.h b/core/fxcrt/cfx_string_c_template.h index bc0fe1e0a0..99948e90a0 100644 --- a/core/fxcrt/cfx_string_c_template.h +++ b/core/fxcrt/cfx_string_c_template.h @@ -119,12 +119,12 @@ class CFX_StringCTemplate { FX_STRSIZE GetLength() const { return m_Length; } bool IsEmpty() const { return m_Length == 0; } - UnsignedType GetAt(FX_STRSIZE index) const { + const UnsignedType& operator[](const FX_STRSIZE index) const { ASSERT(index >= 0 && index < GetLength()); return m_Ptr.Get()[index]; } - CharType CharAt(FX_STRSIZE index) const { + const CharType CharAt(const FX_STRSIZE index) const { ASSERT(index >= 0 && index < GetLength()); return static_cast(m_Ptr.Get()[index]); } @@ -164,8 +164,6 @@ class CFX_StringCTemplate { return CFX_StringCTemplate(m_Ptr.Get() + m_Length - count, count); } - UnsignedType operator[](FX_STRSIZE index) const { return GetAt(index); } - bool operator<(const CFX_StringCTemplate& that) const { int result = FXSYS_cmp(reinterpret_cast(m_Ptr.Get()), reinterpret_cast(that.m_Ptr.Get()), diff --git a/core/fxcrt/cfx_widestring.cpp b/core/fxcrt/cfx_widestring.cpp index c2e1e4877d..01fc8c764e 100644 --- a/core/fxcrt/cfx_widestring.cpp +++ b/core/fxcrt/cfx_widestring.cpp @@ -841,12 +841,6 @@ FX_STRSIZE CFX_WideString::Replace(const CFX_WideStringC& pOld, return nCount; } -void CFX_WideString::SetAt(FX_STRSIZE index, wchar_t c) { - ASSERT(index >= 0 && index < GetLength()); - ReallocBeforeWrite(m_pData->m_nDataLength); - m_pData->m_String[index] = c; -} - // static CFX_WideString CFX_WideString::FromLocal(const CFX_ByteStringC& str) { return FromCodePage(str, 0); @@ -886,6 +880,12 @@ CFX_WideString CFX_WideString::FromUTF16LE(const unsigned short* wstr, return result; } +void CFX_WideString::SetAt(FX_STRSIZE index, wchar_t c) { + ASSERT(index >= 0 && index < GetLength()); + ReallocBeforeWrite(m_pData->m_nDataLength); + m_pData->m_String[index] = c; +} + int CFX_WideString::Compare(const wchar_t* lpsz) const { if (m_pData) return wcscmp(m_pData->m_String, lpsz); diff --git a/core/fxcrt/cfx_widestring.h b/core/fxcrt/cfx_widestring.h index b49e898f32..d57e47c62f 100644 --- a/core/fxcrt/cfx_widestring.h +++ b/core/fxcrt/cfx_widestring.h @@ -101,13 +101,11 @@ class CFX_WideString { bool operator<(const CFX_WideString& str) const; - wchar_t GetAt(FX_STRSIZE index) const { + const CharType& operator[](const FX_STRSIZE index) const { ASSERT(index >= 0 && index < GetLength()); return m_pData->m_String[index]; } - wchar_t operator[](FX_STRSIZE index) const { return GetAt(index); } - void SetAt(FX_STRSIZE index, wchar_t c); int Compare(const wchar_t* str) const; diff --git a/core/fxcrt/cfx_widestring_unittest.cpp b/core/fxcrt/cfx_widestring_unittest.cpp index a23763218d..e7206dde06 100644 --- a/core/fxcrt/cfx_widestring_unittest.cpp +++ b/core/fxcrt/cfx_widestring_unittest.cpp @@ -10,26 +10,8 @@ #include "testing/gtest/include/gtest/gtest.h" -TEST(fxcrt, WideStringGetAt) { - CFX_WideString short_string(L"a"); - CFX_WideString longer_string(L"abc"); - CFX_WideString embedded_nul_string(L"ab\0c", 4); - -#ifndef NDEBUG - EXPECT_DEATH({ short_string.GetAt(-1); }, ".*"); -#endif - EXPECT_EQ(L'a', short_string.GetAt(0)); -#ifndef NDEBUG - EXPECT_DEATH({ short_string.GetAt(1); }, ".*"); -#endif - EXPECT_EQ(L'c', longer_string.GetAt(2)); - EXPECT_EQ(L'b', embedded_nul_string.GetAt(1)); - EXPECT_EQ(L'\0', embedded_nul_string.GetAt(2)); - EXPECT_EQ(L'c', embedded_nul_string.GetAt(3)); -} - -TEST(fxcrt, WideStringOperatorSubscript) { - CFX_WideString abc(L"abc"); +TEST(fxcrt, WideStringElementAccess) { + const CFX_WideString abc(L"abc"); #ifndef NDEBUG EXPECT_DEATH({ abc[-1]; }, ".*"); #endif @@ -39,21 +21,34 @@ TEST(fxcrt, WideStringOperatorSubscript) { #ifndef NDEBUG EXPECT_DEATH({ abc[4]; }, ".*"); #endif -} -TEST(fxcrt, WideStringSetAt) { - CFX_WideString abc(L"abc"); + CFX_WideString mutable_abc = abc; + EXPECT_EQ(abc.c_str(), mutable_abc.c_str()); + EXPECT_EQ(L'a', mutable_abc[0]); + EXPECT_EQ(L'b', mutable_abc[1]); + EXPECT_EQ(L'c', mutable_abc[2]); + EXPECT_EQ(abc.c_str(), mutable_abc.c_str()); #ifndef NDEBUG - EXPECT_DEATH({ abc.SetAt(-1, L'd'); }, ".*"); + EXPECT_DEATH({ mutable_abc.SetAt(-1, L'd'); }, ".*"); + EXPECT_EQ(L"abc", abc); #endif - abc.SetAt(0, L'd'); - EXPECT_EQ(L"dbc", abc); - abc.SetAt(1, L'e'); - EXPECT_EQ(L"dec", abc); - abc.SetAt(2, L'f'); - EXPECT_EQ(L"def", abc); + const wchar_t* c_str = abc.c_str(); + mutable_abc.SetAt(0, L'd'); + EXPECT_EQ(c_str, abc.c_str()); + EXPECT_NE(c_str, mutable_abc.c_str()); + EXPECT_EQ(L"abc", abc); + EXPECT_EQ(L"dbc", mutable_abc); + + mutable_abc.SetAt(1, L'e'); + EXPECT_EQ(L"abc", abc); + EXPECT_EQ(L"dec", mutable_abc); + + mutable_abc.SetAt(2, L'f'); + EXPECT_EQ(L"abc", abc); + EXPECT_EQ(L"def", mutable_abc); #ifndef NDEBUG - EXPECT_DEATH({ abc.SetAt(3, L'g'); }, ".*"); + EXPECT_DEATH({ mutable_abc.SetAt(3, L'g'); }, ".*"); + EXPECT_EQ(L"abc", abc); #endif } @@ -780,25 +775,7 @@ TEST(fxcrt, WideStringCFromVector) { EXPECT_EQ(nullptr, cleared_string.raw_str()); } -TEST(fxcrt, WideStringCGetAt) { - CFX_WideStringC short_string(L"a"); - CFX_WideStringC longer_string(L"abc"); - CFX_WideStringC embedded_nul_string(L"ab\0c", 4); - -#ifndef NDEBUG - EXPECT_DEATH({ short_string.GetAt(-1); }, ".*"); -#endif - EXPECT_EQ(L'a', static_cast(short_string.GetAt(0))); -#ifndef NDEBUG - EXPECT_DEATH({ short_string.GetAt(1); }, ".*"); -#endif - EXPECT_EQ(L'c', static_cast(longer_string.GetAt(2))); - EXPECT_EQ(L'b', static_cast(embedded_nul_string.GetAt(1))); - EXPECT_EQ(L'\0', static_cast(embedded_nul_string.GetAt(2))); - EXPECT_EQ(L'c', static_cast(embedded_nul_string.GetAt(3))); -} - -TEST(fxcrt, WideStringCOperatorSubscript) { +TEST(fxcrt, WideStringCElementAccess) { CFX_WideStringC abc(L"abc"); #ifndef NDEBUG EXPECT_DEATH({ abc[-1]; }, ".*"); diff --git a/core/fxcrt/xml/cxml_parser.cpp b/core/fxcrt/xml/cxml_parser.cpp index 4d9e3dc469..ebd5873b33 100644 --- a/core/fxcrt/xml/cxml_parser.cpp +++ b/core/fxcrt/xml/cxml_parser.cpp @@ -171,7 +171,7 @@ void CXML_Parser::SkipLiterals(const CFX_ByteStringC& str) { int32_t i = 0, iLen = str.GetLength(); do { while (m_dwIndex < m_dwBufferSize) { - if (str.GetAt(i) != m_pBuffer[m_dwIndex++]) { + if (str[i] != m_pBuffer[m_dwIndex++]) { i = 0; continue; } -- cgit v1.2.3