[PATCH] D128750: [C++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
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:
> 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.
By the way, I just became aware of something I missed, and this could totally work here.

We don't have 'canonical expressions', but we do have profiling based on the canonical form of an expression, eg see the `Expr::Profile` method, it has a `Canonical` argument to ask for this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128750/new/

https://reviews.llvm.org/D128750