[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:3285
   }
-  if (const TypedefType *TypedefT =
-          dyn_cast<TypedefType>(FromFPT->getReturnType())) {
-    TypedefNameDecl *TD = TypedefT->getDecl();
+  if (const auto *TypedefT = dyn_cast<TypedefType>(FromFPT->getReturnType())) {
+    const TypedefNameDecl *TD = TypedefT->getDecl();
----------------
rsmith wrote:
> (Preparing for D112374)
Good catch, thanks :)


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1385-1388
+    P = P.getUnqualifiedType();
     //     - If A is a cv-qualified type, A is replaced by the cv-unqualified
     //       version of A.
+    A = A.getUnqualifiedType();
----------------
rsmith wrote:
> Do we need to use `Context.getUnqualifiedArrayType` here, for cases like:
> ```
> typedef const int CInt;
> // Partial ordering should deduce `T = int` from both parameters here,
> // even though `T = const int` might make more sense for the first one.
> template<int N> void f(CInt (&)[N], int*);
> template<int N, typename T> void f(T (&)[N], T*);
> CInt arr[5];
> void g() { f(arr, arr); }
> ```
> ? I think `getUnqualifiedType()` on `CInt[N]` will not remove the indirect `const` qualifier.
I see, that is true, if P and A could be sugared here, we would need `getUnqualifiedArrayType`.

But this issue had been sidestepped because we were not handling sugar in the PartialOrdering case, because that was only used in the isAtLeastAsSpecialized path where we were still using the canonical type, as we don't care about what types where actually deduced there.

I just changed it to not use the canonical type anymore and took this change.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1546-1568
+  QualType CanP = P.getCanonicalType();
+  // If the parameter type is not dependent, there is nothing to deduce.
+  if (!P->isDependentType()) {
+    if (TDF & TDF_SkipNonDependent)
       return Sema::TDK_Success;
+
+    QualType CanA = A.getCanonicalType();
----------------
rsmith wrote:
> I think we can excise `CanA` and `CanP` entirely here so people aren't tempted to use them and lose sugar.
Nice suggestion!

Though we must keep the last return as conditional, as I did there, otherwise we
break a test case such as the one from PR12132:
```
template<typename S> void fun(const int* const S::* member) {}
struct A { int* x; };
void foo() {
    fun(&A::x);
}
```
When ignoring qualifiers, hasSameUnqualifiedType being false is not enough to deduce a mismatch, so this is the one corner case where we analyze with a non-dependent parameter type.

That is what I tried to explain in that comment, but I have just reworded it to be clearer on this point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216