mizvekov added inline comments.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5511-5539
+ template <typename T1, typename T2,
+ std::enable_if_t<std::is_same<T1, T2>::value, bool> = true>
+ bool operator()(T1 *PS1, T2 *PS2) {
+ return hasEqualTemplateArgumentList(
+ PS1->getTemplateArgsAsWritten()->arguments(),
+ PS2->getTemplateArgsAsWritten()->arguments());
+ }
----------------
mizvekov wrote:
> mizvekov wrote:
> > ychen wrote:
> > > mizvekov wrote:
> > > > I think you are not supposed to use the `TemplateArgsAsWritten` here.
> > > >
> > > > The injected arguments are 'Converted' arguments, and the
> > > > transformation above, by unpacking the arguments, is reversing just a
> > > > tiny part of the conversion process.
> > > >
> > > > It's not very meaningful to canonicalize the arguments as written to
> > > > perform a semantic comparison, as that works well just for some kinds
> > > > of template arguments, like types and templates, but not for other
> > > > kinds in which the conversion process is not trivial.
> > > >
> > > > For example, I think this may fail to compare the same integers written
> > > > in different ways, like `2` vs `1 + 1`.
> > > Indeed. It can happen only when comparing one partial specialization with
> > > another. I think the standard does not require an implementation to deal
> > > with this but we could use the best effort without much overhead. For `2`
> > > vs `1+1` or similar template arguments that are not dependent, we could
> > > assume the equivalence because they wouldn't be in the partial ordering
> > > stage if they're not equivalent. For more complicated cases like `J+2` vs
> > > `J+1+1` where J is NTTP, let's stop trying (match GCC) because the
> > > overhead is a little bit high.
> > But I think the 'TemplateArgs', which are the specialization arguments and
> > are available through `getTemplateArgs()`, are the converted arguments you
> > want here, ie the AsWritten arguments converted against the template.
> >
> > I don't see why you can't just use that.
> >
> > How about we change:
> > ```
> > if (!TemplateArgumentListAreEqual(S.getASTContext())(P1, P2))
> > return nullptr;
> > ```
> >
> > Into:
> >
> > ```
> > {
> > ArrayRef<TemplateArgument> Args1 = P1->getTemplateArgs().asArray(),
> > Args2;
> > if constexpr (IsMoreSpecialThanPrimaryCheck)
> > Args2 = P2->getInjectedTemplateArgs();
> > else
> > Args2 = P2->getTemplateArgs().asArray();
> >
> > if (Args1.size() != Args2.size())
> > return nullptr;
> >
> > for (unsigned I = 0, E = Args1.size(); I < E; ++I) {
> > TemplateArgument Arg2 = Args2[I];
> > // Unlike the specialization arguments, the injected arguments are not
> > // always canonical.
> > if constexpr (IsMoreSpecialThanPrimaryCheck)
> > Arg2 = S.Context.getCanonicalTemplateArgument(Arg2);
> >
> > // We use profile, instead of structural comparison of the arguments,
> > // because canonicalization can't do the right thing for dependent
> > // expressions.
> > llvm::FoldingSetNodeID IDA, IDB;
> > Args1[I].Profile(IDA, S.Context);
> > Arg2.Profile(IDB, S.Context);
> > if (IDA != IDB)
> > return nullptr;
> > }
> > }
> > ```
> >
> > That should work, right?
> Actually, you can even further simplify this.
>
> You can't have two different specializations with the same specialization
> arguments. These arguments are used as the key to unique them anyway.
>
> So simplify my above suggestion to:
> ```
> if constexpr (IsMoreSpecialThanPrimaryCheck) {
> ArrayRef<TemplateArgument> Args1 = P1->getTemplateArgs().asArray(),
> Args2 = P2->getInjectedTemplateArgs();
> if (Args1.size() != Args2.size())
> return nullptr;
>
> for (unsigned I = 0, E = Args1.size(); I < E; ++I) {
> // We use profile, instead of structural comparison of the arguments,
> // because canonicalization can't do the right thing for dependent
> // expressions.
> llvm::FoldingSetNodeID IDA, IDB;
> Args1[I].Profile(IDA, S.Context);
> // Unlike the specialization arguments, the injected arguments are not
> // always canonical.
> S.Context.getCanonicalTemplateArgument(Args2[I]).Profile(IDB,
> S.Context);
> if (IDA != IDB)
> return nullptr;
> }
> }
> ```
Yet another improvement here is that you can do this check once, when we create
the specialization, and then simply store a `hasSameArgumentsAsPrimaryInjected`
flag.
When we create the specialization, we already profile the specialization
arguments anyway, so it's another computation you can avoid repeating.
Could even avoid repeating the profiling of the injected arguments if there was
justified runtime overhead there, which I doubt, trading that off with
increased memory use.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128750/new/
https://reviews.llvm.org/D128750
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
