[PATCH] D128750: [C++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions

[Matheus Izvekov via Phabricator via cfe-commits]

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