Fix bug with assigning a part of the string to the same string

The fix has several parts:

1. don't free the old string data in ConcatSelf() if we use it as
   source
2. implement assign() using replace() rather than clear() + append()
3. fix replace() to work with replacement strings containing embedded
   NULs and optimize it by using memcpy() instead of byte-wise copy


git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@48302 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

include/wx/stringimpl.h

@@ -416,18 +416,18 @@ public:
     { return *this = str; }
     // same as ` = str[pos..pos + n]
   wxStringImpl& assign(const wxStringImpl& str, size_t pos, size_t n)
-    { clear(); return append(str, pos, n); }
+    { return replace(0, npos, str, pos, n); }
     // same as `= first n (or all if n == npos) characters of sz'
   wxStringImpl& assign(const wxStringCharType *sz)
-    { clear(); return append(sz, wxStrlen(sz)); }
+    { return replace(0, npos, sz, wxStrlen(sz)); }
   wxStringImpl& assign(const wxStringCharType *sz, size_t n)
-    { clear(); return append(sz, n); }
+    { return replace(0, npos, sz, n); }
     // same as `= n copies of ch'
   wxStringImpl& assign(size_t n, wxStringCharType ch)
-    { clear(); return append(n, ch); }
+    { return replace(0, npos, n, ch); }
     // assign from first to last
   wxStringImpl& assign(const_iterator first, const_iterator last)
-    { clear(); return append(first, last); }
+    { return replace(begin(), end(), first, last); }
 
     // first valid index position
   const_iterator begin() const { return m_pchData; }
@@ -478,18 +478,23 @@ public:
   const wxStringCharType* data() const { return m_pchData; }
 
     // replaces the substring of length nLen starting at nStart
-  wxStringImpl& replace(size_t nStart, size_t nLen, const wxStringCharType* sz);
+  wxStringImpl& replace(size_t nStart, size_t nLen, const wxStringCharType* sz)
+    { return replace(nStart, nLen, sz, npos); }
     // replaces the substring of length nLen starting at nStart
   wxStringImpl& replace(size_t nStart, size_t nLen, const wxStringImpl& str)
-    { return replace(nStart, nLen, str.c_str()); }
+    { return replace(nStart, nLen, str.c_str(), str.length()); }
     // replaces the substring with nCount copies of ch
-  wxStringImpl& replace(size_t nStart, size_t nLen, size_t nCount, wxStringCharType ch);
+  wxStringImpl& replace(size_t nStart, size_t nLen,
+                        size_t nCount, wxStringCharType ch)
+    { return replace(nStart, nLen, wxStringImpl(nCount, ch)); }
     // replaces a substring with another substring
   wxStringImpl& replace(size_t nStart, size_t nLen,
-                        const wxStringImpl& str, size_t nStart2, size_t nLen2);
+                        const wxStringImpl& str, size_t nStart2, size_t nLen2)
+    { return replace(nStart, nLen, str.substr(nStart2, nLen2)); }
     // replaces the substring with first nCount chars of sz
   wxStringImpl& replace(size_t nStart, size_t nLen,
                         const wxStringCharType* sz, size_t nCount);
+
   wxStringImpl& replace(iterator first, iterator last, const_pointer s)
     { return replace(first - begin(), last - first, s); }
   wxStringImpl& replace(iterator first, iterator last, const_pointer s,

src/common/stringimpl.cpp

@@ -576,54 +576,45 @@ size_t wxStringImpl::rfind(wxStringCharType ch, size_t nStart) const
 }
 
 wxStringImpl& wxStringImpl::replace(size_t nStart, size_t nLen,
-                                    const wxStringCharType *sz)
+                                    const wxStringCharType *sz, size_t nCount)
 {
-  wxASSERT_MSG( nStart <= length(),
+    // check and adjust parameters
-                _T("index out of bounds in wxStringImpl::replace") );
+    const size_t lenOld = length();
-  size_t strLen = length() - nStart;
-  nLen = strLen < nLen ? strLen : nLen;
 
-  wxStringImpl strTmp;
+    wxASSERT_MSG( nStart <= lenOld,
-  strTmp.reserve(length()); // micro optimisation to avoid multiple mem allocs
+                  _T("index out of bounds in wxStringImpl::replace") );
+    size_t nEnd = nStart + nLen;
+    if ( nEnd > lenOld )
+    {
+        // nLen may be out of range, as it can be npos, just clump it down
+        nLen = lenOld - nStart;
+        nEnd = lenOld;
+    }
 
-  //This is kind of inefficient, but its pretty good considering...
+    if ( nCount == npos )
-  //we don't want to use character access operators here because on STL
+        nCount = wxStrlen(sz);
-  //it will freeze the reference count of strTmp, which means a deep copy
-  //at the end when swap is called
-  //
-  //Also, we can't use append with the full character pointer and must
-  //do it manually because this string can contain null characters
-  for(size_t i1 = 0; i1 < nStart; ++i1)
-      strTmp.append(1, this->c_str()[i1]);
 
-  //its safe to do the full version here because
+    // build the new string from 3 pieces: part of this string before nStart,
-  //sz must be a normal c string
+    // the new substring and the part of this string after nStart+nLen
-  strTmp.append(sz);
+    wxStringImpl tmp;
+    const size_t lenNew = lenOld + nCount - nLen;
+    if ( lenNew )
+    {
+        tmp.AllocBuffer(lenOld + nCount - nLen);
 
-  for(size_t i2 = nStart + nLen; i2 < length(); ++i2)
+        wxStringCharType *dst = tmp.m_pchData;
-      strTmp.append(1, this->c_str()[i2]);
+        memcpy(dst, m_pchData, nStart*sizeof(wxStringCharType));
+        dst += nStart;
 
-  swap(strTmp);
+        memcpy(dst, sz, nCount*sizeof(wxStringCharType));
-  return *this;
+        dst += nCount;
-}
 
-wxStringImpl& wxStringImpl::replace(size_t nStart, size_t nLen,
+        memcpy(dst, m_pchData + nEnd, (lenOld - nEnd)*sizeof(wxStringCharType));
-                                    size_t nCount, wxStringCharType ch)
+    }
-{
-  return replace(nStart, nLen, wxStringImpl(nCount, ch).c_str());
-}
 
-wxStringImpl& wxStringImpl::replace(size_t nStart, size_t nLen,
+    // and replace this string contents with the new one
-                                    const wxStringImpl& str,
+    swap(tmp);
-                                    size_t nStart2, size_t nLen2)
+    return *this;
-{
-  return replace(nStart, nLen, str.substr(nStart2, nLen2));
-}
-
-wxStringImpl& wxStringImpl::replace(size_t nStart, size_t nLen,
-                                    const wxStringCharType* sz, size_t nCount)
-{
-  return replace(nStart, nLen, wxStringImpl(sz, nCount).c_str());
 }
 
 wxStringImpl wxStringImpl::substr(size_t nStart, size_t nLen) const
@@ -709,6 +700,17 @@ bool wxStringImpl::ConcatSelf(size_t nSrcLen,
   if ( nSrcLen > 0 ) {
     wxStringData *pData = GetStringData();
     size_t nLen = pData->nDataLength;
+
+    // take special care when appending part of this string to itself: the code
+    // below reallocates our buffer and this invalidates pszSrcData pointer so
+    // we have to copy it in another temporary string in this case (but avoid
+    // doing this unnecessarily)
+    if ( pszSrcData >= m_pchData && pszSrcData < m_pchData + nLen )
+    {
+        wxStringImpl tmp(pszSrcData, nSrcLen);
+        return ConcatSelf(nSrcLen, tmp.m_pchData, nSrcLen);
+    }
+
     size_t nNewLen = nLen + nSrcLen;
 
     // alloc new buffer if current is too small