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

Sat Sep 12 06:47:44 PDT 2020

njames93 added a comment.

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

> Hmm - I think maybe what I had trouble with was understanding how much this shuold generalize. Should this handle subranges? (if you try to append half of a vector onto itself) and if so, how? Anyone happen to know what's guaranteed by the standard (perhaps only push_back of single elements - that's easier to handle)

>From what I can see insert and append fully support when the range to insert is enclosed by the vector, however assign makes no such promise as it would potentially require allocating more storage even if the container itself doesn't need to grow

>> I haven't added specific test cases for this, but all ADT Tests did run without a hitch under asan
>
> Did they run without a hitch under asan before the change too? I imagine so.

They did but that was only because the test cases don't grow multiple times from what I can see

> So probably worth adding specific tests for overlapping - ones that would've failed under asan before this change, but pass with it.
>
> The test updates you have done in the patch so far - they're intended to make the test coverage a bit more robust to have more confidence over the refactoring? Could those test changes be committed first/separately - I assume they're intended to pass before and after the patch? So might be good as a preliminary/separate patch before this one.

The code to make the tests more robust fail without the modifications to small vector as they expose issues for insert moving an already deleted item.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87326