[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());
+  }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128750