[PATCH] D57626: Disallow trivial_abi on a class if all copy and move constructors are deleted

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2953
+  "have bases of non-trivial class types|have virtual bases|"
+  "have __weak fields under ARC|have fields of non-trivial class types}0">;
 
----------------
Quuxplusone wrote:
> ahatanak wrote:
> > Quuxplusone wrote:
> > > nit: "of non-trivial class types" should be "of non-trivial class type" in both places.
> > > 
> > > And I would write "are not move-constructible" rather than "don't have non-deleted copy/move constructors". Double negations aren't non-bad.
> > > 
> > > Actually I would rephrase this as `'trivial_abi' is disallowed on this class because it %select{is not move-constructible|is polymorphic|has a base of non-trivial class type|has a virtual base|has a __weak field|has a field of non-trivial class type}`, i.e., we're not just giving information about "classes" in general, we're talking about "this class" specifically. We could even name the class if we're feeling generous.
> > Does not being move-constructible imply that the class doesn't have a *public* copy or move constructor that isn't deleted? If it does, that is slightly different than saying the copy and move constructors of the class are all deleted. When the users see the message "is not move-constructible", they might think the copy or move constructor that isn't deleted has to be public in order to annotate the class with `trivial_abi`. For `trivial_abi`, a private one is good enough.
> > 
> > I think your other suggestions are all good ideas.
> > Does not being move-constructible imply that the class doesn't have a *public* copy or move constructor that isn't deleted?
> 
> Yeah, I suppose so. Okay.
I changed the message to "its copy constructors and move constructors are all deleted".


================
Comment at: test/SemaObjCXX/attr-trivial-abi.mm:108
+    S18(const S18 &);
+    S18(S18 &&);
+  };
----------------
The diagnostic note printed here  was actually "have fields of non-trivial class types", not "don't have non-deleted copy/move constructors", because the class had explicitly declared copy and move constructors. I removed both declarations to make clang print the latter.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57626