mizvekov added inline comments.
================
Comment at: clang/include/clang/Sema/SemaConcept.h:48-52
+ if (ArgA.getKind() == TemplateArgument::Expression &&
+ ArgB.getKind() == TemplateArgument::Expression &&
+ ArgA.getAsExpr()->getType()->isUndeducedAutoType() &&
+ ArgB.getAsExpr()->getType()->isUndeducedAutoType())
+ continue;
----------------
Why are looking at only the type of the expression?
The expression can be arbitrarily different, but as long as they are both
undeduced auto, that is okay?
================
Comment at: clang/include/clang/Sema/SemaConcept.h:54-68
+ if (ArgA.getKind() == TemplateArgument::Type &&
+ ArgB.getKind() == TemplateArgument::Type)
+ if (const auto *SubstA =
+ ArgA.getAsType()->getAs<SubstTemplateTypeParmType>())
+ if (const auto *SubstB =
+ ArgB.getAsType()->getAs<SubstTemplateTypeParmType>()) {
+ QualType ReplacementA = SubstA->getReplacementType();
----------------
It's a bit odd to find `SubstTemplateTypeParmType` necessary to implement the
semantics of this change.
This is just type sugar we leave behind in the template instantiator to mark
where type replacement happened.
There are several potential issues here:
1) This could be marking a substitution that happened at any template depth. Ie
this could be marking a substitution from an outer template. Does the level not
matter here at all?
2) If the level does matter, you won't be able to reconstitute that easily
without further improvements. See
https://github.com/llvm/llvm-project/issues/55886 for example.
3) A substitution can replace a dependent type for another one, and when that
other gets replaced, we lose track of that because `SubstTemplateTypeParmType`
only holds a canonical underlying type.
----
Leaving that aside, I get the impression you are trying to work around the fact
that when an expression appears in a canonical type, presumably because that
expression is dependent on an NTTP, we can't rely on uniquing anymore to
compare if they are the same type, as we lack in Clang the equivalent concept
of canonicalization for expressions.
But this however is a bit hard to implement. Are we sure the standard requires
this, or can we simply consider these types always different?
================
Comment at: clang/lib/AST/ASTContext.cpp:5149-5151
+ Expr *E = new (*this)
+ DeclRefExpr(*this, NTTP, /*enclosing*/ false, T,
+ Expr::getValueKindForType(NTTPType), NTTP->getLocation());
----------------
================
Comment at: clang/lib/AST/ASTContext.cpp:5154-5156
+ E = new (*this) PackExpansionExpr(
+ NTTPType->isUndeducedAutoType() ? NTTPType : DependentTy, E,
+ NTTP->getLocation(), None);
----------------
I don't know if this change is necessary for this patch, as this looks part of
the workaround in `SemaConcept.h`,
but I think a better way to preserve the type here might be to always use
`NTTPType`, but then add an additional `Dependent` parameter to
`PackExpansionExpr` which can be used to force the expression to be dependent.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:827
+ assert(FoldE->isRightFold() && FoldE->getOperator() == BO_LAnd);
+ E = FoldE->getPattern();
+ }
----------------
If you need to dig down into the pattern, then I think the pattern might also
contain casts and parenthesis which you would need to keep ignoring for the
rest of the code to work.
I would consider adding a test for that.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:9675-9676
- // when comparing template functions.
- if (Cand1.Function && Cand2.Function && Cand1.Function->hasPrototype() &&
- Cand2.Function->hasPrototype()) {
auto *PT1 = cast<FunctionProtoType>(Cand1.Function->getFunctionType());
----------------
Why are these `hasPrototype` checks not needed anymore?
I don't see other changes which would obliviate the need for it. Presumably the
code below is assuming we have them when we perform that `FunctionProtoType`
cast.
Maybe this was either not possible, or we simply never added tests for it.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:1267
BuildDeclRefExpr(NTTP, NTTP->getType(), VK_PRValue, NTTP->getLocation());
- if (!Ref)
- return true;
+ assert(Ref);
ExprResult ImmediatelyDeclaredConstraint = formImmediatelyDeclaredConstraint(
----------------
I don't think the `assert` is even necessary TBH, the function can't return
nullptr.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5247-5248
+ for (unsigned i = 0; i < NumParams1; ++i)
+ if (FD1->getParamDecl(i)->getType().getCanonicalType() !=
+ FD2->getParamDecl(i)->getType().getCanonicalType())
+ return nullptr;
----------------
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5256-5257
+ // than the other.
+ if (TPOC == TPOC_Conversion && FD1->getReturnType().getCanonicalType() !=
+ FD2->getReturnType().getCanonicalType())
+ return nullptr;
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128750/new/
https://reviews.llvm.org/D128750
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
