ChuanqiXu added a comment.
I've read the whole patch. It looks good to me roughly except some style issues
and the 2 places you marked as ` Attn Reviewers`. Let's try to fix them one by
one.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4705
CheckConstraintSatisfaction(
- NamedConcept, {NamedConcept->getConstraintExpr()}, Converted,
+ NamedConcept, {NamedConcept->getConstraintExpr()}, MLTAL,
SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
----------------
I would feel better if we could write:
```
CheckConstraintSatisfaction(
NamedConcept, {NamedConcept->getConstraintExpr()}, {MLTAL},
SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
TemplateArgs->getRAngleLoc()),
Satisfaction)
```
But it looks unimplementable.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:5564
return true;
TemplateArgumentList TemplateArgs(TemplateArgumentList::OnStack,
Converted);
----------------
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:7468-7483
+ // Attn Reviewers: This works and fixes the constraint comparison issues,
+ // but I don't have a good idea why this is, nor if it is the 'right'
+ // thing. I THINK it is pulling off the 'template template' level of the
+ // constraint, but I have no idea if that is the correct thing to do.
+ SmallVector<const Expr *, 3> ConvertedParamsAC;
+ TemplateArgumentList Empty(TemplateArgumentList::OnStack, {});
+ MultiLevelTemplateArgumentList MLTAL{Empty};
----------------
I've spent some time to playing it myself to figure it out. And I found that we
could fix this cleaner we adopt above suggestions. The problem here is that the
instantiation is started half way. But the conversion for the constraint have
been deferred. So here is the problem. I guess there are other similar
problems. But let's fix them after we met them actually.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:60-61
const NamedDecl *D, const TemplateArgumentList *Innermost,
- bool RelativeToPrimary, const FunctionDecl *Pattern) {
+ bool RelativeToPrimary, const FunctionDecl *Pattern, bool LookBeyondLambda,
+ bool IncludeContainingStructArgs) {
// Accumulate the set of template argument lists in this structure.
----------------
erichkeane wrote:
> ChuanqiXu wrote:
> > Would you elaborate more for `LookBeyondLambda` and
> > `IncludeContainingStructArgs`? It confuses me since I couldn't find
> > `Lambda` or `Struct` from the context of use point.
> Sure!
>
> So this function is typically used for 'getting the template instantiation
> args' of the current Declaration (D) for a variety of reasons. In all of
> those cases previously, it would 'stop' looking when it hit a lambda generic
> call operator (see line 157). This would block our ability to get the full
> instantiation tree.
>
> Similarly, it would stop at a containing class-template (as most
> instantiations are done against the parent class template). Unfortunately
> this is sufficient, so the IncludeContainingStructArgs (see line 185) ALSO
> includes those arguments, as they are necessary (since they haven't been
> instantiated in the constraint yet).
I got it. It might be necessary to edit the comment too.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119544/new/
https://reviews.llvm.org/D119544
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
