[PATCH] D99005: [clang] Implement P2266 Simpler implicit move

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1059
+    else if (auto *F = dyn_cast<BlockDecl>(DC))
+      goto unimplemented; // FIXME: get the return type here somehow...
+    else
----------------
rjmccall wrote:
> mizvekov wrote:
> > @rjmccall Hello! Do you have any tips for fixing this one?
> I've always found it annoying that we only store the block type on the `BlockExpr` and not the `BlockDecl`, so if you've got a good reason to change that, I'm not opposed.
> 
> That said, while I'm not an expert in how we do NRVO variable tracking, the code must have some ability to handle an unknown return type when just processing declarations by delaying some of the bookkeeping/analysis until the end of the function.  It's possible but quite uncommon to declare a return type explicitly in a block, so that's the dominant case here.  I'm actually surprised we do any of this tracking when we're just instantiating a variable.
Thanks, your observations have been extremely helpful.

So this whole NRVO tracking thing looks to be in bad shape. This tracker here fails to handle 'auto' types. I don't think this gets fixed up anywhere else.

This is also badly covered by the test suite. The test included with the merge request that introduced this tracking (`CodeGenCXX/nrvo.cpp`) still passes if you either delete this whole block here, or if you just do `Var->setNRVOVariable(D->isNRVOVariable())`.

In fact, almost the whole test suite passes with those changes, the only tests which fail are `CodeGenCXX/matrix-type-builtins.cpp` and `CodeGenCXX/matrix-type-operators.cpp` respectively.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99005