ADT: Avoid using SmallVector::set_size() in SmallString

[dexonsmith]

Summary

Update SmallString::append() to use resize_for_overwrite() instead
of reserve() followed by set_size().

A step toward https://reviews.llvm.org/D115380.

[dblaikie]

llvm/include/llvm/ADT/SmallString.h

79–80

Should this use copy, rather than uninitialized_copy? Technically the chars are already constructed, they're just uninitialized - this code would technically cause the same memory to be reconstructed? (I mean, we're only talking about rerunning pseudo constructors so I /think/ it's technically fine either way - but may be simpler to think about using copy?)

[dexonsmith]

llvm/include/llvm/ADT/SmallString.h

79–80

Hmm... agreed this isn't great. this->uninitialized_copy() will call memcpy in this case, but you can't tell that from here. Would std::copy() call memcpy? Probably, but hard to tell, could depend on std library.

I considered switching to a reserve() plus a loop through append(StringRef), but git-blame told me that this function was added in cf8d19f4fb2ca0eb6b7f8169d1d7ff68ba95d9f5 specifically to avoid successive calls to reserve() that the optimizer can't tell are redundant.

I'm tempted at this point to leave it as-is and remove the "make it private" part of https://reviews.llvm.org/D115380, ultimately leaving set_size() available for subclasses to use. WDYT?

[dblaikie]

llvm/include/llvm/ADT/SmallString.h

79–80

I'd generally be happy with std::copy if that suits - I think it's really OK to rely on a reasonably well-performing standard library implementation.

[dexonsmith]

Thanks for the review! Landed in 4d04526bf48d982055fd625803734b2afded3afb.

llvm/include/llvm/ADT/SmallString.h

77

Moved this to precede the for loop before landing (must have moved it down to make the diff look nicer and failed to rerun tests...).

79–80

Yeah, I was overthinking this. Landed change uses std::copy().

llvm/include/llvm/ADT/SmallString.h

public:
 
   /// Append from a StringRef.
   void append(StringRef RHS) {
     SmallVectorImpl<char>::append(RHS.begin(), RHS.end());
   }
 
   /// Append from a list of StringRefs.
   void append(std::initializer_list<StringRef> Refs) {
-    size_t SizeNeeded = this->size();
+    size_t CurrentSize = this->size();
+    size_t SizeNeeded = CurrentSize;
     for (const StringRef &Ref : Refs)
       SizeNeeded += Ref.size();
-    this->reserve(SizeNeeded);
-    auto CurEnd = this->end();
+    this->resize_for_overwrite(SizeNeeded);
     for (const StringRef &Ref : Refs) {
-      this->uninitialized_copy(Ref.begin(), Ref.end(), CurEnd);
-      CurEnd += Ref.size();
+      std::copy(Ref.begin(), Ref.end(), this->begin() + CurrentSize);
+      CurrentSize += Ref.size();
     }
-    this->set_size(SizeNeeded);
+    assert(CurrentSize == this->size());
   }
 
   /// @}
   /// @name String Comparison
   /// @{
 
   /// Check for string equality.  This is more efficient than compare() when
   /// the relative ordering of inequal strings isn't needed.