[PATCH] D87326: [ADT] Fix reference invalidation when self referencing a SmallVector

[Nathan James via Phabricator via llvm-commits]

njames93 marked 3 inline comments as done.
njames93 added a comment.

In D87326#2272611 <https://reviews.llvm.org/D87326#2272611>, @dblaikie wrote:

>> Use global namespace new inside GrowBuffer.
>
> Why this change? Would've thought op new overloads might be desirable, but I certainly don't know the specifics off-hand right now.

SmallVector seems to use global namespace new for all its operations, so I thought I'd follow the convention there.



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:706
+      std::fill(this->begin(), this->end(), Elt);
+    } else if (NumElts < this->size()) {
+      this->destroy_range(this->begin() + NumElts, this->end());
----------------
dblaikie wrote:
> what was wrong/happening in the absence of these "else" clauses? were tests failing? I guess the fill was happening twice, maybe? That should've failed the tests with the tighter constraints you added, right? Did it?
Technically nothing wrong is happening without this else. In the case where `NumElts == this->size()` it'll just copy to copy assign the whole vector with `Elt` twice. Which while bad for performance it isn't breaching any object lifetime rules. The reason no tests fail is because the tests for SmallVector::assign, just check if the contents of the SmallVector are correct, not how many times constructors and assignment operators were called.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87326