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

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

dexonsmith added a comment.

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

> Overhead is likely caused by slightly more code needing to be emitted. However there is a glaring hole here that SmallVector isn't very smart about how it should take paramaters for calls like push_back.
> For small trivial types it makes sense to take those by value and in those instances the current implementation of just calling grow would have no issue about reference invalidation as there no longer a reference. 
> Downside is it would require copying most of the small vector implementation code for this special case, kind of like what currently happens for `SmallVectorTemplateBase` with trivially copyable types.
>
> Given that a lot of use cases of SmallVector seem to be for storing pointers or as the base class for SmallString, this would remove this specific overhead in these cases.

That seems really valuable, and could be committed separately/ahead of the rest of this patch. I don't think you need to split the template; you can just:

  using ParamT = std::conditional<should_take_by_value<T>, T, const T&>;
  void push_back(ParamT Val);

Note also https://reviews.llvm.org/D84293, which proposes adding an assertion when growing is unsafe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87326