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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
