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

[James Player via Phabricator via llvm-commits]

jplayer-nv added a comment.

In D115443#3252648 <https://reviews.llvm.org/D115443#3252648>, @dblaikie wrote:

> 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 generally agree with everything you're saying.  Just wanted to point out that calling `resize_for_overwrite()` in template code can be worked around with a helper template that has a POD specialization:

https://godbolt.org/z/5s5srhx4b

I do agree that keeping `resize_for_overwrite()` available for non-pod types simplifies templated callers by avoiding this extra layer, but the desired effect is still possible when `resize_for_overwrite()` is only callable on PODs.

Another argument to disable for non-POD is code evolution.  Suppose you write proper code around a `SmallVector` of a POD type (call it `SmallVector<Foo>`).  You use `resize_for_overwrite()` like you're supposed to.  Later someone adds a constructor to `Foo`... suddenly your nice efficient code is now less efficient because of a very natural code evolution.

Leaving `resize_for_overwrite()` as an alias to `resize()` in that case silently continues to function with unnecessary overhead (assuming there's a follow-on init loop).  If this evolution becomes a compile failure, then the writer of the `Foo` constructor must deal with all the old `resize_for_overwrite()` callsites (hopefully code them as efficiently as possible).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115443