[libc++] [LIBCXX-DEBUG-FIXME] Fix an iterator-invalidation issue in string::assign.

[• Quuxplusone]

Summary

This appears to be a bug in our string::assign: when assigning into
a longer string, from a shorter snippet of itself, we invalidate
iterators before doing the copy. We should invalidate them afterward.
Also drive-by improve the formatting of a function header.

[Mordante]

SGTM except for a minor question. Do you know why the debug iterator CI job fails?

libcxx/include/string

1787

I like this change!

2362

Is this change really required? We don't use our own iterators in this case.
I'm not objecting against this change.

[• Quuxplusone]

libcxx/include/string

2362

I believe this change is not required; but yeah, it seemed like a good idea for consistency.
The CI job was failing because D101003 hadn't landed in main yet when it kicked off; I'll rebase to prove it's OK.

[• Quuxplusone]

poke buildkite

[Mordante]

Thanks for rebasing, LGTM.

libcxx/include/string

 #if _LIBCPP_DEBUG_LEVEL == 2
     __get_db()->__invalidate_all(this);
 #endif
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 inline
 void
-basic_string<_CharT, _Traits, _Allocator>::__invalidate_iterators_past(size_type
-#if _LIBCPP_DEBUG_LEVEL == 2
-                                                                        __pos
-#endif
-                                                                      )
+basic_string<_CharT, _Traits, _Allocator>::__invalidate_iterators_past(size_type __pos)
 {
 #if _LIBCPP_DEBUG_LEVEL == 2
     __c_node* __c = __get_db()->__find_c_and_lock(this);
     if (__c)
     {
         const_pointer __new_last = __get_pointer() + __pos;
         for (__i_node** __p = __c->end_; __p != __c->beg_; )
         {
             --__p;
             const_iterator* __i = static_cast<const_iterator*>((*__p)->__i_);
             if (__i->base() > __new_last)
             {
                 (*__p)->__c_ = nullptr;
                 if (--__c->end_ != __p)
                     _VSTD::memmove(__p, __p+1, (__c->end_ - __p)*sizeof(__i_node*));
             }
         }
         __get_db()->unlock();
     }
+#else
+    (void)__pos;
 #endif // _LIBCPP_DEBUG_LEVEL == 2
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 inline
 basic_string<_CharT, _Traits, _Allocator>::basic_string()
     _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value)
      : __r_(__default_init_tag(), __default_init_tag())

 basic_string<_CharT, _Traits, _Allocator>::assign(size_type __n, value_type __c)
 {
     size_type __cap = capacity();
     if (__cap < __n)
     {
         size_type __sz = size();
         __grow_by(__cap, __n - __cap, __sz, 0, __sz);
     }
-    else
-        __invalidate_iterators_past(__n);
     value_type* __p = _VSTD::__to_address(__get_pointer());
     traits_type::assign(__p, __n, __c);
     traits_type::assign(__p[__n], value_type());
     __set_size(__n);
+    __invalidate_iterators_past(__n);
     return *this;
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>&
 basic_string<_CharT, _Traits, _Allocator>::operator=(value_type __c)
 {
     pointer __p;
basic_string<_CharT, _Traits, _Allocator>::assign(_ForwardIterator __first, _ForwardIterator __last)
     if (__string_is_trivial_iterator<_ForwardIterator>::value &&
         (__cap >= __n || !__addr_in_range(*__first)))
     {
         if (__cap < __n)
         {
             size_type __sz = size();
             __grow_by(__cap, __n - __cap, __sz, 0, __sz);
         }
-        else
-            __invalidate_iterators_past(__n);
         pointer __p = __get_pointer();
         for (; __first != __last; ++__first, ++__p)
             traits_type::assign(*__p, *__first);
         traits_type::assign(*__p, value_type());
         __set_size(__n);
+        __invalidate_iterators_past(__n);
     }
     else
     {
         const basic_string __temp(__first, __last, __alloc());
         assign(__temp.data(), __temp.size());
     }
     return *this;
 }

libcxx/test/std/strings/basic.string/string.modifiers/string_assign/iterator.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-// XFAIL: LIBCXX-DEBUG-FIXME
-
 // <string>
 
 // template<class InputIterator>
 //   basic_string& assign(InputIterator first, InputIterator last);
 
 #include <string>
 #include <cassert>