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

[Nathan James via Phabricator via llvm-commits]

njames93 added inline comments.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:321
+  using ValueParamT =
+      typename std::conditional<sizeof(T) <= 16, T, const T &>::type;
+
----------------
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*)
```



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


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

https://reviews.llvm.org/D91467