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

Yuanfang Chen via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Sep 18 09:33:00 PDT 2022


ychen added inline comments.


================
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:
> ychen wrote:
> > ychen wrote:
> > > 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.
> > Looking more closely, `T1`/`T2` is mostly returned by `ASTContext::getTemplateSpecializationType` and for injected template specialization, always unqualified   https://github.com/llvm/llvm-project/blob/52dce8900c46d5842a021619537ede598983dfde/clang/include/clang/AST/Type.h#L5431-L5432 , it does not seem possible to have aliase template involved? I could've missed something though.
> Yeah you are right, I missed that all the uses of this function, those types either come from an Injected TST, or a dependent TST created on the spot.
> 
> But yeah, the change to a simple `cast` instead of `castAs` makes this more clear.
> 
> Otherwise, if there could have been any sugar node on top of the TST, that sugar could have easily been an alias TST,
> and I think it's very unlikely one wouldn't care, as they are very different things.
> 
> So I tend to view these as potential bugs, a simple `getAs` and `castAs` on TSTs, which does not check or keeps digging through alias templates is highly suspicious :)
> 
> Thanks for taking a look at this though!
Thanks for the explanation. Yeah, `castAs` looks misleading indeed. I wasn't aware of the subtle difference between `cast` and `castAs`. BTW, are you comfortable accepting this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133683



More information about the cfe-commits mailing list