rjmccall added a subscriber: fhahn. rjmccall 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 ---------------- mizvekov wrote: > 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. I'm surprised that this has any impact on matrix types; pinging @fhahn in case he understands what the interaction might be. It looks like we're assuming that the parse-time analysis of NRVO candidates is correct except for possibly needing adjustment for dependent expressions / return types. The parse-time analysis seems to work by keeping track of NRVO candidates on `Scope`; since we don't push meaningful `Scope`s during template instantiation, that won't work, so we have to trust the parse-time analysis. But the parse-time analysis can also be overly conservative, mostly because of `if constexpr`; that might be enough of a corner case to not merit something better, though. @rsmith is the expert here, though. At any rate, it should be *correct* to handle non-function cases as if the return type were dependent, the same way it presumably works for functions with deduced return types. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99005/new/ https://reviews.llvm.org/D99005 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
