[PATCH] D133683: [c++] implements tentative DR1432 for partial ordering of function template

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

@mizvekov Thanks for taking a look.



================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5195-5196
+    for (int i = 0, e = std::min(NumParams1, NumParams2); i < e; ++i) {
+      QualType T1 = FD1->getParamDecl(i)->getType().getDesugaredType(Context);
+      QualType T2 = FD2->getParamDecl(i)->getType().getDesugaredType(Context);
+      auto *TST1 = dyn_cast<TemplateSpecializationType>(T1);
----------------
mizvekov wrote:
> This should work, as all the properties you are testing below are present on the canonical type.
Gotta admit that I was confused by `DesugaredType` and `CanonicalType`.  But since you pointed it out, I could see the differences now. The `CanonicalType` could be sugared, and desugaring is not necessary here. Thanks.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5455-5456
     if (!ClangABICompat15) {
-      // Consider this a fix for CWG1432. Similar to the fix for CWG1395.
       auto *TST1 = T1->castAs<TemplateSpecializationType>();
       auto *TST2 = T2->castAs<TemplateSpecializationType>();
+      const TemplateArgument &TA1 = TST1->template_arguments().back();
----------------
mizvekov wrote:
> This is a bug, T1 and T2 are not in general canonical types for this function, and this castAs can pick up an alias template, and these should never participate in deduction.
> 
> (Also, can you please add a test for that?)
> 
> You should not need to keep sugar here, everything being tested below is present on the canonical type, so this is simple to solve, as suggested.
I see. I'll add a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133683