[PATCH] D91467: ADT: Take small enough, trivially copyable T by value in SmallVector

Wed Nov 18 17:45:50 PST 2020

dexonsmith added a comment.

In D91467#2404305 <https://reviews.llvm.org/D91467#2404305>, @mehdi_amini wrote:

> Can you make more clear in the description the motivation for this patch? Is this mostly about iterator invalidation? Is this expected to make code safer?

Oh, I did this because I happened upon this comment in https://reviews.llvm.org/D87326:

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.

I can update the description to be more clear if we decide I should commit this.

> I'm not sure about making the mental model a bit harder by having different iterator invalidation behavior implicitly based on T, WDYT?

I'm not sure either, but I think it's worth doing if it unblocks @njames93's broader fix for iterator invalidation (at which point the mental model is simple again). @njames93 , WDYT?

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

https://reviews.llvm.org/D91467