[PATCH] D115382: ADT: Avoid using SmallVector::set_size() in SmallString

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

Thanks for the review! Landed in 4d04526bf48d982055fd625803734b2afded3afb <https://reviews.llvm.org/rG4d04526bf48d982055fd625803734b2afded3afb>.



================
Comment at: llvm/include/llvm/ADT/SmallString.h:76
       SizeNeeded += Ref.size();
-    this->reserve(SizeNeeded);
-    auto CurEnd = this->end();
+    size_t CurrentSize = SizeNeeded;
+    this->resize_for_overwrite(SizeNeeded);
----------------
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...).


================
Comment at: llvm/include/llvm/ADT/SmallString.h:79
     for (const StringRef &Ref : Refs) {
-      this->uninitialized_copy(Ref.begin(), Ref.end(), CurEnd);
-      CurEnd += Ref.size();
+      this->uninitialized_copy(Ref.begin(), Ref.end(),
+                               this->begin() + CurrentSize);
----------------
dblaikie wrote:
> dexonsmith wrote:
> > dblaikie wrote:
> > > 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?)
> > 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?
> 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.
Yeah, I was overthinking this. Landed change uses `std::copy()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115382/new/

https://reviews.llvm.org/D115382