[PATCH] D93779: ADT: Fix reference invalidation in SmallVector::push_back and single-element insert

Wed Jan 13 19:47:43 PST 2021

dblaikie added a comment.

In D93779#2497131 <https://reviews.llvm.org/D93779#2497131>, @dexonsmith wrote:

> In D93779#2497112 <https://reviews.llvm.org/D93779#2497112>, @dblaikie wrote:
>
>> In D93779#2497099 <https://reviews.llvm.org/D93779#2497099>, @dexonsmith wrote:
>>
>>> Reverted in 56d1ffb927d03958a7a31442596df749264a7792 <https://reviews.llvm.org/rG56d1ffb927d03958a7a31442596df749264a7792> due to an error on Windows:
>>> http://lab.llvm.org:8011/#/builders/127/builds/4489/steps/7/logs/stdio
>>> if it's obvious I'll recommit in a minute, but if anyone has ideas I'd welcome them.
>>
>> Guess maybe something in the `std::is_same<std::remove_const_t<std::remove_reference_t<ArgType>>, T>::value` isn't working quite right on MSVC? (could test that with godbolt, maybe) - is it needed? Since it's on both insert_one_maybe_copy and there's an implementation detail/doesn't need to protect from violations of that constraint (admittedly I'm proposing removing it because maybe MSVC is violating it... )? And instead SFINAE/enable_if only on the TakesParamByValue part?
>
> Thanks for looking; I think you're right. The constraint was a trick to allow use of SFINAE since `TakesParamByValue` didn't depend on the template parameter so I can't just remove it, I need a different solution...
>
> ...but I've found one, and it's a bit cleaner too. Instead of the indirection through `insert_one_maybe_copy`, I've added a forwarding function defined in the POD vs. non-POD base classes called `forward_value_param`.
>
> I'll commit the new version once I confirm `check-llvm` passes locally.

Yeah, looks nice/simpler to me!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93779