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

[James Player via Phabricator via llvm-commits]

jplayer-nv added a comment.

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

How about the other direction?  You treat a class as non-trivially copyable expecting a constructor call in `resize_for_overwrite()`, but you actually get back uninitialized data because the class is trivially copyable in some compiler implementation.

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

I agree that's a bug, but no one should be calling `resize_for_overwrite()` without follow-on initialization of the new elements.  The proposed change makes `resize_for_overwrite()` identical to `resize()` in the case of a non-trivially copyable class, which is a non-value add.  So why not `static_assert()` in case of a non-trivially copyable `value_type`?

`resize_for_overwrite()` enables users to write fast code... it is inherently bug-prone if misused.  There's a reason why there's no comparable member function on `std::vector`.  So to me, having `resize_for_overwrite()` sounds like it's breaking the components of `resize()` into

1. buffer growth + back element pointer / size update
2. Initialization / construction; done manually... optimized in calling context.

If there are type_trait scenarios where `resize_for_overwrite()` does both 1. and 2., it may as well not exist in those contexts (maybe it should be SFINAE'd out or just `static_assert()`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115443