[PATCH] D128745: [c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering of variadic template partial specialization or function template

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I admit that I'm feeling a bit out of my element with all the template machinery involved, so I've reviewed as best I'm able (so if any of my questions seem odd to you, push back on them if you want).



================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5184
 
-  // FIXME: This mimics what GCC implements, but doesn't match up with the
-  // proposed resolution for core issue 692. This area needs to be sorted out,
----------------
ychen wrote:
> ychen wrote:
> > aaron.ballman wrote:
> > > ychen wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > We tend not to use top-level const on locals in the project as a matter of style.
> > > > > Does GCC still implement it this way?
> > > > > 
> > > > > One concern I have is that this will be an ABI breaking change, and I'm not certain how disruptive it will be. If GCC already made that change, it may be reasonable for us to also break it without having to add ABI tags or something special. But if these changes diverge us from GCC, that may require some extra effort to mitigate.
> > > > Unfortunately, GCC is still using the old/non-conforming behavior. https://clang.godbolt.org/z/5K4916W71. What is the usual way to mitigate this?
> > > You would use the `ClangABI` enumeration to alter behavior between ABI versions: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/LangOptions.h#L174 like done in: https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/DeclCXX.cpp#L929
> > Looking at how `ClangABI` is currently used, it looks like it is for low-level object layout ABI compatibility. For library/language ABI compatibility, maybe we should not use `ClangABI`? Looking at https://reviews.llvm.org/rG86a1b135f0c67a022ef628f092c6691892752876, I guess the practice is just committed and see? If it breaks important or many existing libraries, just revert or add compiler options?
> > I guess the practice is just committed and see? If it breaks important or many existing libraries, just revert or add compiler options?
> 
> I'm fairly optimistic considering `check-runtimes` passes. 
> Looking at how ClangABI is currently used, it looks like it is for low-level object layout ABI compatibility. For library/language ABI compatibility, maybe we should not use ClangABI? 

I don't know that there's any specific guidance that we only use it for object layout ABI compatibility; we have used it for behavior beyond layout in the past: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclCXX.cpp#L9786

> I guess the practice is just committed and see? If it breaks important or many existing libraries, just revert or add compiler options?

Somewhat, yes. We aim for full ABI compatibility with GCC and consider ABI differences to be bugs (generally speaking). If we break some important existing library, that bug is critical to get fixed ASAP, but any ABI different can potentially bite users.

I would say: if matching the old ABI requires convoluting the implementation too much, we can skip it; otherwise, we should probably match GCC's ABI just to avoid concerns.

CC @rsmith in case he has different opinions as code owner.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1110
+  //   Ai is ignored;
+  if (PartialOrdering && ArgIdx + 1 == NumArgs &&
+      isa<PackExpansionType>(Args[ArgIdx]))
----------------
Why are you checking `ArgIdx + 1 == NumArgs` here? It looks like you're only handling the case where the pack is the last argument, but packs can appear anywhere, right?


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2488
+        bool XHasMoreArg = X.pack_size() > Y.pack_size();
+        if (!(XHasMoreArg && X.pack_elements().back().isPackExpansion()) &&
+            !(!XHasMoreArg && Y.pack_elements().back().isPackExpansion()))
----------------
ychen wrote:
> FYI: `isSameTemplateArg` is for class/variable partial ordering deduction. The corresponding check for function template deduction is skipped by intention. See https://reviews.llvm.org/rG86a1b135f0c67a022ef628f092c6691892752876 and https://github.com/llvm/llvm-project/blob/e6f1f062457c928c18a88c612f39d9e168f65a85/clang/lib/Sema/SemaTemplateDeduction.cpp#L5064-L5066.
Same question here about looking at just the last element.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5399
+};
+
+/// Returns the more specialized template specialization between T1/P1 and
----------------
You should end the anonymous namespace here per our coding style guidelines.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5423-5424
+template <typename TemplateLikeDecl, typename PrimaryDel,
+          bool IsMoreSpecialThanPrimaryCheck =
+              !std::is_same<TemplateLikeDecl, PrimaryDel>::value>
+static TemplateLikeDecl *
----------------
Can't this be a local constexpr variable instead of an NTTP, or are there reasons you want to allow callers to be able to override that?


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5426-5427
+static TemplateLikeDecl *
+getMoreSpecialized(Sema &S, QualType T1, QualType T2, TemplateLikeDecl *P1,
+                   PrimaryDel *P2, TemplateDeductionInfo &Info) {
+  bool Better1 = isAtLeastAsSpecializedAs(S, T1, T2, P2, Info);
----------------
Curiosity question -- can you make `P1` and `P2` be pointers to `const` without many other viral changes to existing code?


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5444
+    if (TST1->getNumArgs()) {
+      const TemplateArgument &TA1 = TST1->template_arguments().back();
+      if (TA1.getKind() == TemplateArgument::Pack) {
----------------
Again, should you be looking at all the packs here and not just trailing ones?


================
Comment at: clang/www/cxx_dr_status.html:4197
     <td>Partial ordering of variadic class template partial specializations</td>
-    <td class="none" align="center">No</td>
+    <td class="full" align="center">Clang 15</td>
   </tr>
----------------
You should use `unreleased` as the class for these because Clang 15 hasn't been release yet (they'll switch over to `full` once we release).


================
Comment at: clang/www/cxx_dr_status.html:8187
     <td>Partial ordering of variadic templates reconsidered</td>
-    <td class="none" align="center">Unknown</td>
+    <td class="full" align="center">Clang 15</td>
   </tr>
----------------
Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128745