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

[Akira Hatanaka via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf466f0beda59: Disallow trivial_abi on a class if all copy 
and move constructors are deleted (authored by ahatanak).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57626

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaObjCXX/attr-trivial-abi.mm

Index: clang/test/SemaObjCXX/attr-trivial-abi.mm
===
--- clang/test/SemaObjCXX/attr-trivial-abi.mm
+++ clang/test/SemaObjCXX/attr-trivial-abi.mm
@@ -10,23 +10,23 @@
   int a;
 };
 
-struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}} expected-note {{is polymorphic}}
   virtual void m();
 };
 
 struct S3_2 {
   virtual void m();
-} __attribute__((trivial_abi)); // expected-warning {{'trivial_abi' cannot be applied to 'S3_2'}}
+} __attribute__((trivial_abi)); // expected-warning {{'trivial_abi' cannot be applied to 'S3_2'}} expected-note {{is polymorphic}}
 
 struct S4 {
   int a;
 };
 
-struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}} expected-note {{has a virtual base}}
 };
 
 struct __attribute__((trivial_abi)) S9 : public S4 {
@@ -36,19 +36,19 @@
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}}
+struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}}
+struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}} expected-note {{has a __weak field}}
   __weak id a[2];
 };
 
-struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}} expected-note {{has a field of a non-trivial class type}}
   S6 a;
 };
 
-struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}} expected-note {{has a field of a non-trivial class type}}
   S6 a[2];
 };
 
@@ -66,7 +66,7 @@
 S10<__weak id> p2;
 
 template<>
-struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}}
+struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
@@ -90,8 +90,39 @@
 S16 s16;
 
 template
-struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}}
+struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
 S17 s17;
+
+namespace deletedCopyMoveConstructor {
+  struct __attribute__((trivial_abi)) CopyMoveDeleted { // expected-warning {{'trivial_abi' cannot be applied to 'CopyMoveDeleted'}} expected-note {{copy constructors and move constructors are all deleted}}
+CopyMoveDeleted(const CopyMoveDeleted &) = delete;
+CopyMoveDeleted(CopyMoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S18 { // expected-warning {{'trivial_abi' cannot be applied to 'S18'}} expected-note {{copy constructors and move constructors are all deleted}}
+CopyMoveDeleted a;
+  };
+
+  struct __attribute__((trivial_abi)) CopyDeleted {
+CopyDeleted(const CopyDeleted &) = delete;
+CopyDeleted(CopyDeleted &&) = default;
+  };
+
+  struct __attribute__((trivial_abi)) MoveDeleted {
+MoveDeleted(const MoveDeleted &) = default;
+MoveDeleted(MoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S19 { // expected-warning {{'trivial_abi' cannot be applied to 'S19'}} expected-note {{copy constructors and move constructors are all deleted}}
+CopyDeleted a;
+MoveDeleted b;
+  };
+
+  // This 

[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 a comment.

Yes, I'm still interested in pursuing this. I'll rebase the patch and commit it 
if I don't get any more feedback.


Repository:
  rG LLVM Github Monorepo

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

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 269633.
ahatanak added a comment.

Rebase patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57626

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaObjCXX/attr-trivial-abi.mm

Index: clang/test/SemaObjCXX/attr-trivial-abi.mm
===
--- clang/test/SemaObjCXX/attr-trivial-abi.mm
+++ clang/test/SemaObjCXX/attr-trivial-abi.mm
@@ -10,23 +10,23 @@
   int a;
 };
 
-struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}} expected-note {{is polymorphic}}
   virtual void m();
 };
 
 struct S3_2 {
   virtual void m();
-} __attribute__((trivial_abi)); // expected-warning {{'trivial_abi' cannot be applied to 'S3_2'}}
+} __attribute__((trivial_abi)); // expected-warning {{'trivial_abi' cannot be applied to 'S3_2'}} expected-note {{is polymorphic}}
 
 struct S4 {
   int a;
 };
 
-struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}} expected-note {{has a virtual base}}
 };
 
 struct __attribute__((trivial_abi)) S9 : public S4 {
@@ -36,19 +36,19 @@
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}}
+struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}}
+struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}} expected-note {{has a __weak field}}
   __weak id a[2];
 };
 
-struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}} expected-note {{has a field of a non-trivial class type}}
   S6 a;
 };
 
-struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}} expected-note {{has a field of a non-trivial class type}}
   S6 a[2];
 };
 
@@ -66,7 +66,7 @@
 S10<__weak id> p2;
 
 template<>
-struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}}
+struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
@@ -90,8 +90,39 @@
 S16 s16;
 
 template
-struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}}
+struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
 S17 s17;
+
+namespace deletedCopyMoveConstructor {
+  struct __attribute__((trivial_abi)) CopyMoveDeleted { // expected-warning {{'trivial_abi' cannot be applied to 'CopyMoveDeleted'}} expected-note {{copy constructors and move constructors are all deleted}}
+CopyMoveDeleted(const CopyMoveDeleted &) = delete;
+CopyMoveDeleted(CopyMoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S18 { // expected-warning {{'trivial_abi' cannot be applied to 'S18'}} expected-note {{copy constructors and move constructors are all deleted}}
+CopyMoveDeleted a;
+  };
+
+  struct __attribute__((trivial_abi)) CopyDeleted {
+CopyDeleted(const CopyDeleted &) = delete;
+CopyDeleted(CopyDeleted &&) = default;
+  };
+
+  struct __attribute__((trivial_abi)) MoveDeleted {
+MoveDeleted(const MoveDeleted &) = default;
+MoveDeleted(MoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S19 { // expected-warning {{'trivial_abi' cannot be applied to 'S19'}} expected-note {{copy constructors and move constructors are all deleted}}
+CopyDeleted a;
+MoveDeleted b;
+  };
+
+  // This is fine since the move constructor isn't deleted.
+  struct __attribute__((trivial_abi)) S20 {
+int & // a member of rvalue reference type deletes the copy constructor.
+  };
+}
Index: 

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.
Herald added a subscriber: ributzka.

Are you still interested in pursuing this? This change looks good to me if so.


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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 186083.
ahatanak marked 4 inline comments as done.
ahatanak added a comment.

Improve diagnostic messages.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57626

Files:
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclCXX.cpp
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- test/SemaObjCXX/attr-trivial-abi.mm
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -10,23 +10,23 @@
   int a;
 };
 
-struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}} expected-note {{is polymorphic}}
   virtual void m();
 };
 
 struct S3_2 {
   virtual void m();
-} __attribute__((trivial_abi)); // expected-warning {{'trivial_abi' cannot be applied to 'S3_2'}}
+} __attribute__((trivial_abi)); // expected-warning {{'trivial_abi' cannot be applied to 'S3_2'}} expected-note {{is polymorphic}}
 
 struct S4 {
   int a;
 };
 
-struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}} expected-note {{has a virtual base}}
 };
 
 struct __attribute__((trivial_abi)) S9 : public S4 {
@@ -36,19 +36,19 @@
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}}
+struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}}
+struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}} expected-note {{has a __weak field}}
   __weak id a[2];
 };
 
-struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}} expected-note {{has a field of a non-trivial class type}}
   S6 a;
 };
 
-struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}} expected-note {{has a field of a non-trivial class type}}
   S6 a[2];
 };
 
@@ -66,7 +66,7 @@
 S10<__weak id> p2;
 
 template<>
-struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}}
+struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
@@ -90,8 +90,39 @@
 S16 s16;
 
 template
-struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}}
+struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}} expected-note {{has a __weak field}}
   __weak id a;
 };
 
 S17 s17;
+
+namespace deletedCopyMoveConstructor {
+  struct __attribute__((trivial_abi)) CopyMoveDeleted { // expected-warning {{'trivial_abi' cannot be applied to 'CopyMoveDeleted'}} expected-note {{copy constructors and move constructors are all deleted}}
+CopyMoveDeleted(const CopyMoveDeleted &) = delete;
+CopyMoveDeleted(CopyMoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S18 { // expected-warning {{'trivial_abi' cannot be applied to 'S18'}} expected-note {{copy constructors and move constructors are all deleted}}
+CopyMoveDeleted a;
+  };
+
+  struct __attribute__((trivial_abi)) CopyDeleted {
+CopyDeleted(const CopyDeleted &) = delete;
+CopyDeleted(CopyDeleted &&) = default;
+  };
+
+  struct __attribute__((trivial_abi)) MoveDeleted {
+MoveDeleted(const MoveDeleted &) = default;
+MoveDeleted(MoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S19 { // expected-warning {{'trivial_abi' cannot be applied to 'S19'}} expected-note {{copy constructors and move constructors are all deleted}}
+CopyDeleted a;
+MoveDeleted b;
+  };
+
+  // This is fine since the move constructor isn't deleted.
+  struct __attribute__((trivial_abi)) S20 {
+int & // a member of rvalue reference type deletes the copy constructor.
+  };
+}
Index: 

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone 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">;
 

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.


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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D57626#1382391 , @Quuxplusone wrote:

> I admit that this `lock_guard` example is contrived and generally ill-advised 
> ,
>  but its ill-advisedness seems like a higher-level concern that shouldn't be 
> "enforced" by fiddling with the rules of [[trivial_abi]], so I hope that's 
> not what's going on here.


`[[trivial_abi]]` (at least right now) only affects whether user-provided 
special member functions are considered to be trivial for ABI purposes. A class 
whose copy and move constructor are both deleted is not passed in registers by 
the ABI; that has nothing to do with triviality, so it's unaffected by 
`[[trivial_abi]]` as currently specified. We could "fiddle with the rules of 
[[trivial_abi]]" to *make* that work (that's what the previous approach for 
this case did), but as you note, this is an ill-advised case, and fiddling with 
the rules to give it special behavior doesn't seem like worthwhile complexity. 
Our design intent was to produce a diagnostic if `[[trivial_abi]]` is specified 
on a non-template class that we can't actually pass in registers; this patch 
fixes a hole in our implementation of that design by adding a missing 
diagnostic.


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

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone 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">;
 

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.



Comment at: lib/Sema/SemaDeclCXX.cpp:7886
+return false;
+  };
+

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.


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

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 185157.
ahatanak added a comment.

Add a note diagnostic to inform the user of the reason for not allowing 
annotating a class with `trivial_abi`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57626

Files:
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclCXX.cpp
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- test/SemaObjCXX/attr-trivial-abi.mm
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -10,23 +10,23 @@
   int a;
 };
 
-struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}} expected-note {{have __weak fields under ARC}}
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}} expected-note {{are polymorphic}}
   virtual void m();
 };
 
 struct S3_2 {
   virtual void m();
-} __attribute__((trivial_abi)); // expected-warning {{'trivial_abi' cannot be applied to 'S3_2'}}
+} __attribute__((trivial_abi)); // expected-warning {{'trivial_abi' cannot be applied to 'S3_2'}} expected-note {{are polymorphic}}
 
 struct S4 {
   int a;
 };
 
-struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}} expected-note {{have virtual bases}}
 };
 
 struct __attribute__((trivial_abi)) S9 : public S4 {
@@ -36,19 +36,19 @@
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}}
+struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}} expected-note {{have __weak fields under ARC}}
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}}
+struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}} expected-note {{have __weak fields under ARC}}
   __weak id a[2];
 };
 
-struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}} expected-note {{have fields of non-trivial class types}}
   S6 a;
 };
 
-struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}} expected-note {{have fields of non-trivial class types}}
   S6 a[2];
 };
 
@@ -66,7 +66,7 @@
 S10<__weak id> p2;
 
 template<>
-struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}}
+struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}} expected-note {{have __weak fields under ARC}}
   __weak id a;
 };
 
@@ -90,8 +90,41 @@
 S16 s16;
 
 template
-struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}}
+struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}} expected-note {{have __weak fields under ARC}}
   __weak id a;
 };
 
 S17 s17;
+
+namespace deletedCopyMoveConstructor {
+  struct __attribute__((trivial_abi)) CopyMoveDeleted { // expected-warning {{'trivial_abi' cannot be applied to 'CopyMoveDeleted'}} expected-note {{"don't have non-deleted copy/move constructors}}
+CopyMoveDeleted(const CopyMoveDeleted &) = delete;
+CopyMoveDeleted(CopyMoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S18 { // expected-warning {{'trivial_abi' cannot be applied to 'S18'}} expected-note {{"don't have non-deleted copy/move constructors}}
+CopyMoveDeleted a;
+S18(const S18 &);
+S18(S18 &&);
+  };
+
+  struct __attribute__((trivial_abi)) CopyDeleted {
+CopyDeleted(const CopyDeleted &) = delete;
+CopyDeleted(CopyDeleted &&) = default;
+  };
+
+  struct __attribute__((trivial_abi)) MoveDeleted {
+MoveDeleted(const MoveDeleted &) = default;
+MoveDeleted(MoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S19 { // expected-warning {{'trivial_abi' cannot be applied to 'S19'}} expected-note {{"don't have non-deleted copy/move constructors}}
+CopyDeleted a;
+MoveDeleted b;
+  };
+
+  // This is fine since the move constructor isn't deleted.
+  struct __attribute__((trivial_abi)) S20 {
+

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Richard felt that this was an odd special case, and I was happy to use the old 
language-designer's dodge of banning something today so that we can decide to 
allow it tomorrow.  This isn't SFINAE-able.

...of course, if it's just a *warning* that this isn't allowed, that dodge 
doesn't quite work.


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

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

Can you give more intuition on why classes with no copy/move operations should 
be forced non-trivial-abi?  Let's take this specific example:

  struct [[clang::trivial_abi]] lock_guard {
  mutex *m;
  explicit lock_guard(mutex *m) : m(m) { m->lock(); }
  ~lock_guard() { m->unlock(); }
  lock_guard(const lock_guard&) = delete;
  lock_guard(lock_guard&&) = delete;
  };
  
  void foo(lock_guard g) { ... }
  void bar() { mutex m; foo(lock_guard()); }

With C++17 "guaranteed copy elision," there's no reason this code would 
//need// copy/move operations. But equally I can't see any reason that `g` 
should not be passed in a register when possible — it's just a pointer, after 
all.
I admit that this `lock_guard` example is contrived and generally ill-advised 
,
 but its ill-advisedness seems like a higher-level concern that shouldn't be 
"enforced" by fiddling with the rules of [[trivial_abi]], so I hope that's not 
what's going on here.


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

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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaDeclCXX.cpp:7886
+  if (!HasNonDeletedCopyOrMoveConstructor()) {
+PrintDiagAndRemoveAttr();
+return;

This is not a very useful diagnostic.  We have several different reasons why we 
reject the attribute, some of which are pretty subtle, and it's not reasonable 
to expect users to puzzle it out.  At the very least we can tell them the 
immediate cause for all of these rejections.


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

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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak created this revision.
ahatanak added reviewers: rsmith, rjmccall, aaron.ballman.
ahatanak added a project: clang.
Herald added subscribers: dexonsmith, jkorous.

Instead of forcing the class to be passed in registers, which was what r350920 
did, issue a warning and inform the user that the attribute cannot be used.

For more background, see this discussion: 
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190128/259907.html

This fixes PR39683.

rdar://problem/47308221


Repository:
  rC Clang

https://reviews.llvm.org/D57626

Files:
  include/clang/Basic/AttrDocs.td
  lib/Sema/SemaDeclCXX.cpp
  test/SemaObjCXX/attr-trivial-abi.mm


Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- test/SemaObjCXX/attr-trivial-abi.mm
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -95,3 +95,36 @@
 };
 
 S17 s17;
+
+namespace deletedCopyMoveConstructor {
+  struct __attribute__((trivial_abi)) CopyMoveDeleted { // expected-warning 
{{'trivial_abi' cannot be applied to 'CopyMoveDeleted'}}
+CopyMoveDeleted(const CopyMoveDeleted &) = delete;
+CopyMoveDeleted(CopyMoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S18 { // expected-warning 
{{'trivial_abi' cannot be applied to 'S18'}}
+CopyMoveDeleted a;
+S18(const S18 &);
+S18(S18 &&);
+  };
+
+  struct __attribute__((trivial_abi)) CopyDeleted {
+CopyDeleted(const CopyDeleted &) = delete;
+CopyDeleted(CopyDeleted &&) = default;
+  };
+
+  struct __attribute__((trivial_abi)) MoveDeleted {
+MoveDeleted(const MoveDeleted &) = default;
+MoveDeleted(MoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S19 { // expected-warning 
{{'trivial_abi' cannot be applied to 'S19'}}
+CopyDeleted a;
+MoveDeleted b;
+  };
+
+  // This is fine since the move constructor isn't deleted.
+  struct __attribute__((trivial_abi)) S20 {
+int & // a member of rvalue reference type deletes the copy constructor.
+  };
+}
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -7868,6 +7868,25 @@
 RD.dropAttr();
   };
 
+  // Ill-formed if the copy and move constructors are deleted.
+  auto HasNonDeletedCopyOrMoveConstructor = [&]() {
+if (RD.needsImplicitCopyConstructor() &&
+!RD.defaultedCopyConstructorIsDeleted())
+  return true;
+if (RD.needsImplicitMoveConstructor() &&
+!RD.defaultedMoveConstructorIsDeleted())
+  return true;
+for (const CXXConstructorDecl *CD : RD.ctors())
+  if (CD->isCopyOrMoveConstructor() && !CD->isDeleted())
+return true;
+return false;
+  };
+
+  if (!HasNonDeletedCopyOrMoveConstructor()) {
+PrintDiagAndRemoveAttr();
+return;
+  }
+
   // Ill-formed if the struct has virtual functions.
   if (RD.isPolymorphic()) {
 PrintDiagAndRemoveAttr();
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -2566,6 +2566,7 @@
 Attribute ``trivial_abi`` has no effect in the following cases:
 
 - The class directly declares a virtual base or virtual methods.
+- The class doesn't have a copy or move constructor that isn't deleted.
 - The class has a base class that is non-trivial for the purposes of calls.
 - The class has a non-static data member whose type is non-trivial for the 
purposes of calls, which includes:
 


Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- test/SemaObjCXX/attr-trivial-abi.mm
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -95,3 +95,36 @@
 };
 
 S17 s17;
+
+namespace deletedCopyMoveConstructor {
+  struct __attribute__((trivial_abi)) CopyMoveDeleted { // expected-warning {{'trivial_abi' cannot be applied to 'CopyMoveDeleted'}}
+CopyMoveDeleted(const CopyMoveDeleted &) = delete;
+CopyMoveDeleted(CopyMoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S18 { // expected-warning {{'trivial_abi' cannot be applied to 'S18'}}
+CopyMoveDeleted a;
+S18(const S18 &);
+S18(S18 &&);
+  };
+
+  struct __attribute__((trivial_abi)) CopyDeleted {
+CopyDeleted(const CopyDeleted &) = delete;
+CopyDeleted(CopyDeleted &&) = default;
+  };
+
+  struct __attribute__((trivial_abi)) MoveDeleted {
+MoveDeleted(const MoveDeleted &) = default;
+MoveDeleted(MoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S19 { // expected-warning {{'trivial_abi' cannot be applied to 'S19'}}
+CopyDeleted a;
+MoveDeleted b;
+  };
+
+  // This is fine since the move constructor isn't deleted.
+  struct __attribute__((trivial_abi)) S20 {
+int & // a member of rvalue reference type deletes the copy constructor.
+  };
+}
Index: lib/Sema/SemaDeclCXX.cpp