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

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

dexonsmith added inline comments.


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115382