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

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson requested changes to this revision.
royjacobson added a comment.
This revision now requires changes to proceed.

Thanks for working on this! I like the approach, but two comments:

I would like more tests for the different forms of template argument deduction. Some suggestions for things to test: template template arguments, 'auto' in abbreviated function templates, argument packs, non-type template parameters. We should at least test that we find the more constrained function when the parameters are equal.

I'm also a bit concerned that we're deviating from the standard here w.r.t. to the 'reordering' checks for reversed candidates. I support this - I think your version makes more sense than what the standard says to do here - but if someone could check with the committee (I don't have reflector access myself) that we're not breaking some important use case by doing it this way, I think it's a good idea.



================
Comment at: clang/docs/ReleaseNotes.rst:134-138
 - Overload resolution for constrained function templates could use the partial
   order of constraints to select an overload, even if the parameter types of
   the functions were different. It now diagnoses this case correctly as an
   ambiguous call and an error. Fixes
   `Issue 53640 <https://github.com/llvm/llvm-project/issues/53640>`_.
----------------
You can remove this bullet from when I partially implemented this paper :)


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:7810
+                                           TemplateParameterList *Old) {
+  SmallVector<unsigned, 4> IterIdxs(Old->size());
+  std::iota(IterIdxs.begin(), IterIdxs.end(), 0);
----------------
Consider adding


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5119
-  while (--NumParams > 0) {
-    if (Function->getParamDecl(NumParams - 1)->isParameterPack())
-      return false;
----------------
Curious, why was this removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128750