Inline basic_string::erase for fastpath where __n == npos

[mvels]

Summary

This change checks for the case where people want to erase a string to the end, i.e., __n == npos, and inlines the call if so.

This also demonstrates keeping the ABI intact for V1, but inlining the erase() method for unstable.

[mvels]

Make inlining subject to both unstable ABI and V2

[mvels]

Added clarifying comments

[ldionne]

You'll need to update the ABI lists for the unstable ABI to account for the addition of __erase_external. Try running ninja check-cxx-abilist.

Lurkers: Martijn and I spoke at length and we decided to try and have two fully separate lists of exported symbols for the stable and unstable ABIs. We think this will make it clearer what's exported in what configuration, etc. In particular, I didn't like the fact that in the stable ABI this patch started exporting __erase_external even though it would never actually be used from the dylib (cause otherwise it's an ABI break for back-deployment). Martijn nicely agreed to prototype a patch that splits the two lists out, and we'll see which way we go based on that patch.

libcxx/include/string

3004

'externally instantiated' ?

3039

In what way is this unstable?

[mvels]

The external template list changes (split into explicit V1 and UNSTABLE versions) are in https://reviews.llvm.org/D74870

I'll rebase this change on that one (assuming arc phabricator figures out me doing so)

[mvels]

• Fix external template list

[mvels]

libcxx/include/string

3039

I agree this comment is confusing, as it details 'here is ABI detail' which we deliberately keep as a separate concern in __string.

So I removed the comment, if you have suggestions for a comment that may be helpful for a future reader or editor that may not be aware of this function potentially available for inlining under unstable, I welcome alternatives.

[mvels]

Is there a way I can easily tell phabricator that this is baselined against https://reviews.llvm.org/D74870 ?

I admit I don't understand phabricator well, or the mysterious ways it works.

On my local repo I rebased my branch, which works like a charm as that is ..... basic git.
Unfortunately arc diff then updates the wrong review, so I had to fix up both this one and D74870,

I also figured setting 'parent revision' would establish this (not so much).

Should I RTFM? :)

[mvels]

• Fix external template list + typo

[smeenai]

In D73743#1883768, @mvels wrote:

Is there a way I can easily tell phabricator that this is baselined against https://reviews.llvm.org/D74870 ?

I admit I don't understand phabricator well, or the mysterious ways it works.

On my local repo I rebased my branch, which works like a charm as that is ..... basic git.
Unfortunately arc diff then updates the wrong review, so I had to fix up both this one and D74870,

I also figured setting 'parent revision' would establish this (not so much).

Should I RTFM? :)

Try arc diff HEAD^.

We should just adjust the .arcconfig for LLVM to do this automatically ... I'll put up a patch.

[mvels]

Inline basic_string::erase for fastpath where __n == npos

Some renames, rediffed on existing ABI lists

[mvels]

• Cleaned up return types
• Udpated ABI list

[mvels]

PTAL

[mvels]

• Cosmetics: un-delete empty line

[smeenai]

In D73743#1883819, @smeenai wrote:

In D73743#1883768, @mvels wrote:

Is there a way I can easily tell phabricator that this is baselined against https://reviews.llvm.org/D74870 ?

I admit I don't understand phabricator well, or the mysterious ways it works.

On my local repo I rebased my branch, which works like a charm as that is ..... basic git.
Unfortunately arc diff then updates the wrong review, so I had to fix up both this one and D74870,

I also figured setting 'parent revision' would establish this (not so much).

Should I RTFM? :)

Try arc diff HEAD^.

We should just adjust the .arcconfig for LLVM to do this automatically ... I'll put up a patch.

D74990

[EricWF]

LGTM.

[EricWF]

libcxx/include/string

1579

Nit: I was fond of using external to denote that a function was designed as such.

3008

I don't think any operation in this function can throw. Should we mark it noexcept?

[mvels]

Small twiddles from review

[leonardchan]

Hi, I believe this patch is causing the following undefined symbol error we're seeing in our 2 stage mac builders:

[367/3568] Linking CXX executable bin/llvm-tblgen
FAILED: bin/llvm-tblgen 
: && /b/s/w/ir/k/recipe_cleanup/clangINjhKi/llvm_build_dir/./bin/clang++  -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -flto -fdebug-prefix-map=/b/s/w/ir/k/recipe_cleanup/clangINjhKi/llvm_build_dir/tools/clang/stage2-bins=../recipe_cleanup/clangINjhKi/llvm_build_dir/tools/clang/stage2-bins -fdebug-prefix-map=/b/s/w/ir/k/llvm-project/= -no-canonical-prefixes -O3 -DNDEBUG -Wl,-search_paths_first -Wl,-headerpad_max_install_names -nostdlib++ /b/s/w/ir/k/cipd/lib/libc++.a -flto -Wl,-lto_library -Wl,/b/s/w/ir/k/recipe_cleanup/clangINjhKi/llvm_build_dir/./lib/libLTO.dylib    -Wl,-dead_strip utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmMatcherEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterInst.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/Attributes.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CallingConvEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeEmitterGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenDAGPatterns.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenHwModes.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenInstruction.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenMapTable.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenRegisters.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenSchedule.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenTarget.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherOpt.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcher.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DFAEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DFAPacketizerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DisassemblerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/ExegesisEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/FastISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/FixedLenDecoderEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/GICombinerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/GlobalISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InfoByHwMode.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InstrInfoEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InstrDocsEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/IntrinsicEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptParserEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptRSTEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/PredicateExpander.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/PseudoLoweringEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RISCVCompressInstEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RegisterBankEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RegisterInfoEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SDNodeProperties.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SearchableTableEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SubtargetEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SubtargetFeatureInfo.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/TableGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/Types.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86DisassemblerTables.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86EVEX2VEXTablesEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86FoldTablesEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86ModRMFilters.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86RecognizableInstr.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/WebAssemblyDisassemblerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CTagsEmitter.cpp.o  -o bin/llvm-tblgen  -Wl,-rpath,@loader_path/../lib lib/libLLVMSupport.a lib/libLLVMTableGen.a lib/libLLVMTableGenGlobalISel.a lib/libLLVMSupport.a /usr/lib/libz.dylib -lm lib/libLLVMDemangle.a && :
Undefined symbols for architecture x86_64:
  "std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> >::__erase_external_with_move(unsigned long, unsigned long)", referenced from:
      (anonymous namespace)::AsmWriterEmitter::EmitPrintInstruction(llvm::raw_ostream&) in lto.o
      UnescapeString(std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> >&) in lto.o
ld: symbol(s) not found for architecture x86_64
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

Could you take a look and send out a fix or revert? Thanks.

Builder: https://luci-milo.appspot.com/p/fuchsia/builders/prod/clang-mac-x64/b8887357725525428704

libcxx/include/__string

#define _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_Func, _CharType) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find_last_of(value_type const*, size_type, size_type) const) \
   _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__grow_by(size_type, size_type, size_type, size_type, size_type, size_type)) \
   _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__grow_by_and_replace(size_type, size_type, size_type, size_type, size_type, size_type, value_type const*)) \
   _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::push_back(value_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::append(size_type, value_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::rfind(value_type, size_type) const) \
   _Func(_LIBCPP_FUNC_VIS const basic_string<_CharType>::size_type basic_string<_CharType>::npos) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::assign(size_type, value_type)) \
-  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::erase(size_type, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__erase_external_with_move(size_type, size_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::append(basic_string const&, size_type, size_type)) \
   _Func(_LIBCPP_FUNC_VIS int basic_string<_CharType>::compare(value_type const*) const) \
   _Func(_LIBCPP_FUNC_VIS int basic_string<_CharType>::compare(size_type, size_type, value_type const*) const) \
   _Func(_LIBCPP_FUNC_VIS _CharType& basic_string<_CharType>::at(size_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::assign(value_type const*)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find(value_type const*, size_type, size_type) const) \
   _Func(_LIBCPP_FUNC_VIS int basic_string<_CharType>::compare(size_type, size_type, basic_string const&, size_type, size_type) const) \
   _Func(_LIBCPP_FUNC_VIS int basic_string<_CharType>::compare(size_type, size_type, value_type const*, size_type) const) \

libcxx/include/string

void __grow_by(size_type __old_cap, size_type __delta_cap, size_type __old_sz,
                    size_type __n_copy,  size_type __n_del,     size_type __n_add = 0);
     void __grow_by_and_replace(size_type __old_cap, size_type __delta_cap, size_type __old_sz,
                                size_type __n_copy,  size_type __n_del,
                                size_type __n_add, const value_type* __p_new_stuff);
 
     _LIBCPP_INLINE_VISIBILITY
     void __erase_to_end(size_type __pos);
 
+    // __erase_external_with_move is invoked for erase() invocations where
+    // `n ~= npos`, likely requiring memory moves on the string data.
+    void __erase_external_with_move(size_type __pos, size_type __n);
+
     _LIBCPP_INLINE_VISIBILITY
     void __copy_assign_alloc(const basic_string& __str)
         {__copy_assign_alloc(__str, integral_constant<bool,
                       __alloc_traits::propagate_on_container_copy_assignment::value>());}
 
     _LIBCPP_INLINE_VISIBILITY
     void __copy_assign_alloc(const basic_string& __str, true_type)
         {

 basic_string<_CharT, _Traits, _Allocator>&
 basic_string<_CharT, _Traits, _Allocator>::replace(const_iterator __i1, const_iterator __i2, size_type __n, value_type __c)
 {
     return replace(static_cast<size_type>(__i1 - begin()), static_cast<size_type>(__i2 - __i1), __n, __c);
 }
 
 // erase
 
+// 'externally instantiated' erase() implementation, called when __n != npos.
+// Does not check __pos against size()
 template <class _CharT, class _Traits, class _Allocator>
-basic_string<_CharT, _Traits, _Allocator>&
-basic_string<_CharT, _Traits, _Allocator>::erase(size_type __pos, size_type __n)
+void
+basic_string<_CharT, _Traits, _Allocator>::__erase_external_with_move(
+    size_type __pos, size_type __n)
 {
-    size_type __sz = size();
-    if (__pos > __sz)
-        this->__throw_out_of_range();
     if (__n)
     {
+        size_type __sz = size();
         value_type* __p = _VSTD::__to_address(__get_pointer());
         __n = _VSTD::min(__n, __sz - __pos);
         size_type __n_move = __sz - __pos - __n;
         if (__n_move != 0)
             traits_type::move(__p + __pos, __p + __pos + __n, __n_move);
         __sz -= __n;
         __set_size(__sz);
         __invalidate_iterators_past(__sz);
         traits_type::assign(__p[__sz], value_type());
     }
-    return *this;
+}
+
+template <class _CharT, class _Traits, class _Allocator>
+basic_string<_CharT, _Traits, _Allocator>&
+basic_string<_CharT, _Traits, _Allocator>::erase(size_type __pos,
+                                                 size_type __n) {
+  if (__pos > size()) this->__throw_out_of_range();
+  if (__n == npos) {
+    __erase_to_end(__pos);
+  } else {
+    __erase_external_with_move(__pos, __n);
+  }
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 inline
 typename basic_string<_CharT, _Traits, _Allocator>::iterator
 basic_string<_CharT, _Traits, _Allocator>::erase(const_iterator __pos)
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2
     _LIBCPP_ASSERT(__get_const_db()->__find_c_from_i(&__pos) == this,
         "string::erase(iterator) called with an iterator not"
         " referring to this string");