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

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

dexonsmith added inline comments.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:321
+  using ValueParamT =
+      typename std::conditional<sizeof(T) <= 16, T, const T &>::type;
+
----------------
silvas wrote:
> njames93 wrote:
> > In relation to my comment about insert, it would be a lot easier to implement if this condition was extracted out into a static constexpr variable
> > ```lang=c++
> > static constexpr bool IsPushByVal = sizeof(T) <= 16;
> > ```
> > 
> > A second point is maybe this should be based loosely on the target. Though I'm not super sure on that, WDYT?
> > ```lang=c++
> > sizeof(T) <= 2 * sizeof(void*)
> > ```
> > 
> +1, I think that basing it on sizeof(void*) probably makes sense.
BTW, the updated patch takes both of these suggestions.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:594-598
     // If we just moved the element we're inserting, be sure to update
     // the reference.
     const T *EltPtr = &Elt;
     if (I <= EltPtr && EltPtr < this->end())
       ++EltPtr;
----------------
dexonsmith wrote:
> njames93 wrote:
> > This code is redundant if Elt is passed by value, however I don't think all(or any) compilers are smart enough to see that, wrapping this around an if block dependent on whether Elt is passed by ref would in all likelihood be a win. Same goes for the other call to insert below.
> Good point; I’ll update on Monday. 
Updated patch covers this.


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

https://reviews.llvm.org/D91467