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

[Benjamin Kramer via Phabricator via llvm-commits]

bkramer added a comment.

In D115443#3242421 <https://reviews.llvm.org/D115443#3242421>, @jplayer-nv wrote:

> 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).

But is this really a problem? If `std::is_trivially_copyable` isn't aggressive you get redundant initializations, but it's not a bug. ABI-wise this should also be fine because no struct layout is affected.

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

I somewhat agree, but I'm really concerned about cases like

  {
    SmallVector<std::unique_ptr<T>> v;
    v.resize_for_overwrite(42);
  }
  <crash in destructor>

And also operator= no longer working. Maybe it's not an issue in reality ...

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

No, this function is only used on PODs. I was planning to use it on SmallVectors of SourceLocations in Clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115443