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

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added inline comments.


================
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()))
----------------
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.


================
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,
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128745