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 <rharrison@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>

9 files changed, 76 insertions, 127 deletions

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<char>(short_string.GetAt(0)));
-#ifndef NDEBUG
-  EXPECT_DEATH({ short_string.GetAt(1); }, ".*");
-#endif
-  EXPECT_EQ('c', static_cast<char>(longer_string.GetAt(2)));
-  EXPECT_EQ('b', static_cast<char>(embedded_nul_string.GetAt(1)));
-  EXPECT_EQ('\0', static_cast<char>(embedded_nul_string.GetAt(2)));
-  EXPECT_EQ('c', static_cast<char>(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<CharType>(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<const CharType*>(m_Ptr.Get()),
                            reinterpret_cast<const CharType*>(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<wchar_t>(short_string.GetAt(0)));
-#ifndef NDEBUG
-  EXPECT_DEATH({ short_string.GetAt(1); }, ".*");
-#endif
-  EXPECT_EQ(L'c', static_cast<wchar_t>(longer_string.GetAt(2)));
-  EXPECT_EQ(L'b', static_cast<wchar_t>(embedded_nul_string.GetAt(1)));
-  EXPECT_EQ(L'\0', static_cast<wchar_t>(embedded_nul_string.GetAt(2)));
-  EXPECT_EQ(L'c', static_cast<wchar_t>(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;
       }