ychen added inline comments.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5423-5424
+template <typename TemplateLikeDecl, typename PrimaryDel,
+ bool IsMoreSpecialThanPrimaryCheck =
+ !std::is_same<TemplateLikeDecl, PrimaryDel>::value>
+static TemplateLikeDecl *
----------------
aaron.ballman wrote:
> 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?
Ah, I didn't think of that. The caller has no need to override this.
================
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);
----------------
aaron.ballman wrote:
> Curiosity question -- can you make `P1` and `P2` be pointers to `const`
> without many other viral changes to existing code?
`isAtLeastAsSpecializedAs` uses template instantiations which modify P1/P2.
================
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:
> > 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.
That makes sense to me. Added the ABI compatibility check.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128745/new/
https://reviews.llvm.org/D128745
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
