[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

Mon Jun 14 15:17:21 PDT 2021

mizvekov added a comment.

In D99696#2817126 <https://reviews.llvm.org/D99696#2817126>, @rjmccall wrote:

> When `__block` variables get moved to the heap, they're supposed to be moved if possible, not copied.  It looks like PerformMoveOrCopyInitialization requires NRVO info to be passed in to ever do a move.  Maybe it's being passed in wrong when building `__block` copy expressions in some situation.

Found the culprit, that functionality was implemented in checkEscapingByref, and it was a bit of a shocker that such a thing was just hiding in a function with such an unassuming name :)
Also surprising to find here another user of the two-step overload resolution, that also escaped our radar.
There is a C++ proposal (P2266 <https://reviews.llvm.org/P2266>) to replace that mechanism with something much simpler.

So since this is an ObjectiveC thing, how is the semantics of this interaction defined, just in terms of C++ NRVO?
As this reuses `PerformMoveOrCopyInitialization`, it is also affected by the change going into C++20 where the restriction on only converting constructors was lifted. Was this accidental and nobody noticed?

For this patch I am going to revert to the original behavior, but I think this could be revisited in the future so it gets aligned with the C++2b rules after P2266 <https://reviews.llvm.org/P2266>, which would be just having VarRef be an XValue unconditionally and perform the regular copy initialization.

> Was there really not a test case covering those semantics?

Nope...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696