[PATCH] D128619: [Clang] Implement P0848 (Conditionally Trivial Special Member Functions)

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:494-497
+- Implemented "Conditionally Trivial Special Member Functions" (`P0848 <https://wg21.link/p0848r3>`_).
+  Note: The handling of deleted functions is not yet compliant, as Clang
+  does not implement `DR1496 <https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1496>`_
+  and `DR1734 <https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1734>`_.
----------------
h-vetinari wrote:
> Is that lack of compliance worth a note in `cxx_status`?
I'm not very opinionated about this, but I tend to not significant enough. I mean, 7 years later and only MSVC have even bothered to implement them.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17875
+    return true;
+  if (!Context.hasSameType(M1->getParamDecl(0)->getType(),
+                           M2->getParamDecl(0)->getType()))
----------------
royjacobson wrote:
> shafik wrote:
> > What happens if we have further parameters with default arguments? Unless I am missing something they are still special member functions but the proposal does not seem to cover them.
> That's an excellent question.
> 
> I'm not sure what to do about default arguments. In a context where the additional parameters matter, you're not using them as constructors anymore, right? So why would this affect the type traits?
> On the one hand [over.match.best] is against this idea of comparing constraints when the parameters differ. So also every context where this actually matters the overload resolution would probably be ambiguous anyway?
> 
> @BRevzin, what do you think? Is the wording intentional to include copy/move constructors with default arguments as well?
> 
> I checked with GCC and they seem to handle default arguments separately: https://godbolt.org/z/1ch3M7MjP
> 
FWIW, I filed https://github.com/cplusplus/CWG/issues/110

I'm not on the reflector myself, so I don't know if there's been a follow-up there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128619