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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

It's less safe, yes - but the question is how much less safe do we want to make it. What's the tradeoff of efficiency and safety.

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

It can be/probably is useful for the API to be there, but be a no-op - so that it can be used in generic code (eg: in templates that don't know if they're operating on trivial or non-trivial element types, they call it, then carry on - for non-trivial types it doesn't do anything, for trivial ones it provides some performance improvement opportunities).

I wouldn't be entirely against SFINAE or static_asserting, but I don't think it adds much value - maybe it avoids callers accidentally using the API where it'd be no different than a plain `resize` and confusing readers/themselves about any extra performance benefit. (I'd /generally/ discourage folks from using `resize_for_overwrite` in cases where it's statically known to be no better than `resize` due to possible confusion about construction order, etc - while there are other API situations where I'd encourage use of distinct APIs that aren't any different but maybe make intent more clear (eg: a container with O(1) "length" you could argue that "empty()" is no better than "length() == 0", but I think the intent is beneficial even if it doesn't have any performance impact - but in the case of "resize_for_overwrite" I think it's a nuanced enough API (even in the safer version that's a no-op for non-trivially-constructible types) that it should raise eyebrows when you see it, and should only be seen when necessary so it can continue to be a "read the surrounding code carefully" signal)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115443