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:
> > 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.
It doesn't even need to be a flag to store in each SpecializationDecl, because
there can be only one specialization with the same arguments as the primary
injected arguments, for obvious reasons :D
So probably just a pointer in the TemplateDecl...
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
