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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115382