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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked 2 inline comments as done.
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:
> 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.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:7886
+    return false;
+  };
+
----------------
Quuxplusone wrote:
> How confident are we that this logic is correct?  I ask because I need something similar for my own diagnostic in D50119. If this logic is rock-solid (no lurking corner-case bugs), I should copy it — and/or it should be moved into a helper member function on `CXXRecordDecl` so that other people can call it too.
I think it's correct. The first part looks for implicit constructors that are not deleted, and the for loop looks for the explicitly declared ones that aren't deleted.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57626