ychen marked 4 inline comments as done.
ychen added a comment.
@mizvekov thanks for the detailed review. It helped me a lot in reconsidering
the auto type handling.
================
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;
----------------
mizvekov wrote:
> 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?
In the partial ordering context, the expression is the same explicit template
argument. So we could treat two undeduced autos as equal.
This code is to deal with the fact that, `AuoType` is currently being uniqued
on type constrained, which goes against the spirit of P2113R0 which considers
type constraints only when the two types are equivalent.
I think the more neat way is to unique auto template parameter with the kind of
placeholder type (`auto`, `auto*`, `auto&`, `decltype(auto)`, ...), and its
template parameter depth/index. Then we don't need the workaround here and I
could simplify the code in `SemaTemplateDeduction.cpp` too. WDYT?
================
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();
----------------
mizvekov wrote:
> 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?
> It's a bit odd to find SubstTemplateTypeParmType necessary to implement the
> semantics of this change.
Odd indeed.
> 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.
Yeah, sort of . This workaround is to deal with the fact that `DecltypeType` is
not uniqued. However, the injected template argument for `t` of `template<auto
t>` is `decltype(t)` (which on a side note, might be wrong since `auto` means
using template arg deduct rules; `decltype(auto)` means using `decltype(expr)`
type, let's keep it this way now since this code path is still needed when
Clang starts to support `decltype(auto)` as NTTP type) and concepts partial
ordering rules need to compare these concept template arguments
(https://eel.is/c++draft/temp.constr#atomic-1).
Looking at the motivation why `DecltypeType` is not uniqued
(https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ASTContext.cpp#L5637-L5640),
I think maybe we should unique decltype on the expr to deal with concepts
cleanly. Thoughts?
================
Comment at: clang/lib/AST/ASTContext.cpp:5154-5156
+ E = new (*this) PackExpansionExpr(
+ NTTPType->isUndeducedAutoType() ? NTTPType : DependentTy, E,
+ NTTP->getLocation(), None);
----------------
mizvekov wrote:
> 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.
> I don't know if this change is necessary for this patch, as this looks part
> of the workaround in `SemaConcept.h`,
Yes, it is.
> 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.
Giving it a second thought, I think I could inspect the pattern of the
PackExpansionExpr instead. I'll revert this hunk.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:827
+ assert(FoldE->isRightFold() && FoldE->getOperator() == BO_LAnd);
+ E = FoldE->getPattern();
+ }
----------------
mizvekov wrote:
> 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.
The pattern is fixed as a concept id, like `C<T>`. I couldn't figure out when
parentheses might show up.
(https://eel.is/c++draft/dcl.spec.auto#nt:placeholder-type-specifier). I might
be wrong.
================
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());
----------------
mizvekov wrote:
> 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.
In c++, there will always be a prototype for the function?
================
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(
----------------
mizvekov wrote:
> I don't think the `assert` is even necessary TBH, the function can't return
> nullptr.
Agreed. Removed.
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
