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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D87326#2272934 <https://reviews.llvm.org/D87326#2272934>, @njames93 wrote:

> 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.

Fair enough - thanks for walking me through it.



================
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());
----------------
njames93 wrote:
> 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.
Could you make the tests a bit more rigorous to test for this fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87326