mizvekov 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(); ---------------- ychen wrote: > 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? I hope I answered that in my other comment, but since this is a speculative wording fix, I think it's more reasonable to wait more than a week for other reviewers, specially since folks can be on time off and such. But lacking interest from other folks in reviewing this, yeah sure. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133683/new/ https://reviews.llvm.org/D133683 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits