std::string::replace (iterator, iterator, InputIterator, InputIterator) inserting self incorrect

• Key: STDCXX-170
• Type: Bug
• Status: Reopened
• Priority: Minor
• Assignee: Unassigned
• Reporter: Anton Pevtsov
• Component/s: 21. Strings
• Affects Version/s: 4.1.3, 4.1.4, 4.2.0
• Fix Version/s: 4.3.0

Description

This test fails:

#include <iostream>
#include <string>

static const char* test = "babc";

int main (void)
{
std::string s ("abc");
s.replace (s.begin (), s.begin (), s.begin () + 1, s.begin () + 2);

std::cout << "Expected " << test << " and got " << s << '\n';

return 0;
}

The output is "Expected babc and got aabc".

See details here:
http://mail-archives.apache.org/mod_mbox/incubator-stdcxx-dev/200604.mbox/[e-mail]%3e

Comments

[Martin Sebor]

Here's another test case for the same problem that should work:

$ cat t.cpp && make t && ./t
#include <cassert>
#include <string>

int main ()
{
std::string s ("abc");

s.replace (s.begin (), s.begin (), s.rbegin () + 1, s.rbegin () + 2);

assert ("babc" == s);
}

gcc -c -I/build/sebor/dev/stdlib/include/ansi -D_RWSTDDEBUG -pthreads -D_RWSTD_USE_CONFIG -I/build/sebor/gcc-4.1.0-15s/include -I/build/sebor/dev/stdlib/include -I/build/sebor/dev/stdlib/../rwtest -I/build/sebor/dev/stdlib/../rwtest/include -I/build/sebor/dev/stdlib/tests/include -pedantic -nostdinc++ -g -W -Wall -Wcast-qual -Winline -Wshadow -Wwrite-strings -Wno-long-long t.cpp
gcc t.o -o t -L/build/sebor/gcc-4.1.0-15s/rwtest -lrwtest15s -pthreads -L/build/sebor/gcc-4.1.0-15s/lib -lstd15s -lsupc++ -lm
Assertion failed: "babc" == s, file t.cpp, line 10
Abort (core dumped)

[Martin Sebor]

And another version of the same to show that there is an unbounded set of specializations of the replace() member function template whose arguments may reference elements in *this.

#include <cassert>
#include <string>

int main ()
{
std::string s ("abc");

typedef const unsigned char UChar;
s.replace (s.begin (), s.begin (), (UChar*)&s [1], (UChar*)&s [2]);

assert ("babc" == s);
}

[Farid Zaripov]

The patch is attached.

[Farid Zaripov]

The regression test is attached.

[Farid Zaripov]

Corrected patch is attached.

[Martin Sebor]

I'm not sure we can introduce a string_type local typedef into <string>. Unless there already is a typedef with that name in some scope the name should be private (e.g., _C_string_type).

@ -476,6 +476,8 @@

     typedef _Alloc                                allocator_type;
     typedef _TYPENAME allocator_type::size_type   size_type;
 
+    typedef _STD::basic_string<_CharT, _Traits, _Alloc> string_type;

Can you explain the reinterpret_cast in the hunk below?

@@ -638,14 +653,40 @@

         }
         else {
             // Current reference has enough room.
-            if (__rem)  
+            pointer __tmp = 0;
+
+            if (__n2) {
+                const_reference __ref =
+                    _RWSTD_REINTERPRET_CAST (const_reference, *__first2);
+                const const_pointer __ptr =
+                    _RWSTD_VALUE_ALLOC (_C_value_alloc_type, __s,
+                                        address (__ref));
+                if (__s.data () <= __ptr && __s.data () + __ssize > __ptr) {
+                    __tmp = _RWSTD_VALUE_ALLOC (_C_value_alloc_type, __s,
+                                                allocate (__n2));
+                    for (__d = 0; __d < __n2; __d++)
+                        traits_type::assign (*(__tmp + __d), *__first2++);
+                }
+            }
+
+            if (__rem)

[Farid Zaripov]

The *__first2 expression can be of different type than const_reference.

I.e. from the third example (see comment at 27/Apr/06 12:01 AM), the const_reference is const char&, but *__first2 is unsigned char&.

The allocator<>::address() method accept allocator<>::const_reference type, and to avoid of making the temporary object of char type, assigned with a result of *__first2, and passing the reference to this temporary object in address() method I've used the reinterpret_cast.

The using address() method is not necessary and the following code

 
                const_reference __ref =
                    _RWSTD_REINTERPRET_CAST (const_reference, *__first2);
                const const_pointer __ptr =
                    _RWSTD_VALUE_ALLOC (_C_value_alloc_type, __s,
                                        address (__ref));

could be replaced to

 
                const const_pointer __ptr =
                    &_RWSTD_REINTERPRET_CAST (const_reference, *__first2);

[Farid Zaripov]

The patch committed in trunk thus: http://svn.apache.org/viewvc?rev=629550&view=rev
The regression test added in trunk thus: http://svn.apache.org/viewvc?rev=629551&view=rev

[Farid Zaripov]

The issue will be closed after passing the nightly builds and merging into the 4.2.x branch.

[Farid Zaripov]

The patch merged in 4.2.x branch thus: http://svn.apache.org/viewvc?rev=630259&view=rev
The regression test merged in 4.2.x branch thus: http://svn.apache.org/viewvc?rev=630257&view=rev

[Farid Zaripov]

The current patch causing problems. The issue should be fixed using type traits templates and adding overloads for existing methods. This is not binary compatible changes and issue is deferred to 4.3 release.

The patch partially reverted from trunk thus: http://svn.apache.org/viewvc?rev=637897&view=rev
The patch partially reverted and regression test deleted from branches/4.2.x thus: http://svn.apache.org/viewvc?rev=637898&view=rev

[Travis Vitek]

Attaching patch for review.

The patch stdcxx-170.patch is suitable for 4.2.2. If the range being copied from is a pointer type and overlaps or IterType is not a pointer type, then we make a copy of the source range and then replace using the temporary.

I could create an additional patch that would allow us to avoid creating unnecessary copies of the src range when the iterators are std::string::iterator or std::string::const_iterator types. This would only provide a benefit in debug builds, so I don't see it as being extremely useful.