mizvekov added a comment.
Hello, thanks, I appreciate that you found it helpful!
It's pretty late for me now, I will give this another look tomorrow.
================
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;
----------------
ychen wrote:
> 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?
If you mean a change so that constraints are not part of the canonical type of
undeduced AutoType, that is worth a try.
I can't think right now of a reason why they should be part of it in the first
place.
================
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();
----------------
ychen wrote:
> 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?
I see, there should be no problem with a change to unique a DecltypeType, but
it might not help you, because expressions typically have source location
information embedded in them.
================
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());
----------------
ychen wrote:
> 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?
Yeah that makes the most sense :)
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
