aaronpuchert marked an inline comment as done.
aaronpuchert added inline comments.
================
Comment at: clang/lib/Sema/TreeTransform.h:12162
for (unsigned I = 0, NumParams = NewCallOperator->getNumParams();
I != NumParams; ++I) {
----------------
aaronpuchert wrote:
> 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.
Or maybe default arguments need to instantiated immediately similar to the
situation from DR1484? It says "Within a template declaration ...", which would
seem to apply here. There is no enclosing scope though, so I'm not sure we can
call this a local class.
So I guess we need to instantiate in more cases in `SubstParmVarDecl`, as you
indicated.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76038/new/
https://reviews.llvm.org/D76038
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
