[PATCH] D115443: [ADT] Weaken the initialization condition in SmallVector::resize_for_overwrite

[James Player via Phabricator via llvm-commits]

jplayer-nv added a comment.

> Older MSVC has trouble with is_trivially_copyable

If I recall correctly from https://reviews.llvm.org/D93510, the original problem was not due to `std::is_trivially_copyable` being wrong.  It was just an insufficient condition to select the trivial `OptionalStorage` specialization because there were additional assumptions rolled up into taking that specialization.  The `Optional` bug existed independent of the changes MSVC made to the `std::pair` class (which triggered the compile failure in my build environment).

In theory, we could separate these assumptions into to multiple specializations of the trivial `OptionalStorage` specialization omitting copy/move constructors and cover the entire breadth of `std::is_trivially_copyable` types.

---

**For this review:**

My concern would be the variations in the result of `std::is_trivially_copyable` across different c++ standards.  It's possible that you could have a class considered non-trivially copyable in one compiler and trivially copyable in another.  This means that it becomes unclear whether you can populate the buffer with `std::uninitialized_copy` or you should call `std::copy` (for example).

Personally, I don't think `resize_for_overwrite()` should construct values regardless of type traits.  In that case, `std::uninitialized_copy()` and `std::uninitialized_fill()` would always be appropriate follow-ups.

Are there currently many instances of `resize_for_overwrite()` called for a non-POD type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115443