[PATCH] D76038: PR45000: Use Sema::SetParamDefaultArgument in TransformLambdaExpr

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked 2 inline comments as done.
aaronpuchert added inline comments.


================
Comment at: clang/lib/Sema/TreeTransform.h:12162
 
   for (unsigned I = 0, NumParams = NewCallOperator->getNumParams();
        I != NumParams; ++I) {
----------------
rsmith wrote:
> rsmith wrote:
> > This logic shouldn't even be here in the first place.. this should all be handled by `SubstParmVarDecl` in `SemaTemplateInstantiate`. It's wrong for `TreeTransform` to be assuming that `TransformFunctionProtoType` will have built new function parameters -- it doesn't do so by default, so in the non-template-instantiation case, this is clobbering the default argument information on the original lambda. And in the template instantiation case, this is clobbering the default argument information (including initialization) that `SubstParmVarDecl` already did.
> > 
> > Please try deleting this loop entirely. If there are still problems (and I think there are: I don't think we handle delayed instantiation for default argument in generic lambdas properly), we should address them by changing how we do default argument instantiation in `SubstParmVarDecl`. In particular, where that function calls `SetParamDefaultArgument` after calling `SubstExpr`, we should presumably instead be calling `setUninstantiatedDefaultArg` on the parameter if it's a parameter of a generic lambda (we still need to instantiate the default argument with the template arguments of the actual lambda call operator).
> Hmm, what I wrote above isn't entirely true -- `TreeTransform` does create new parameters by default. But nonetheless, it's the responsibility of `TransformFunctionTypeParam` to set up the default argument, not the responsibility of this code, so we shouldn't be partially overwriting its results here. (Perhaps we should move the substitution into default arguments from `SubstParmVarDecl` into `TreeTransform::TransformFunctionTypeParam`, though I think that can be done separately from this bugfix.)
Thanks for your hints! I was already somewhat suspicious of that loop. Removing it works indeed pretty well, but I'm running into an issue with variable templates. Default arguments are usually only instantiated when needed by a call, which happens in `Sema::CheckCXXDefaultArgExpr`. Now in the following code, the `MultiLevelTemplateArgumentList` returned from `Sema::getTemplateInstantiationArgs` is empty instead of being `[int]`:

```
template <typename T>
int k = [](T x = 0.0) -> int { return x; }();

template int k<int>;
```

Looking at `getTemplateInstantiationArgs`, that's not very surprising: if we are a `DeclContext`, we go through the contexts and collect all template arguments from those, but a variable template is not a context. The variable template case is handled right at the beginning, but only if the declaration itself is a variable template.

Meaning that deep inside the initializer of a variable template, we don't get its template parameters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76038