rjmccall added inline comments.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:7033
+  // member of an ObjC pointer type, except when it has an in-class 
initializer,
+  // it doesn't make the defaulted default constructor defined as deleted.
+  if (!FieldType.hasNonTrivialObjCLifetime() ||
----------------
This comment should be talking about non-trivial ownership rather than just 
general ObjC pointer-ish-ness.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:7036
+      (CSM == Sema::CXXDefaultConstructor && FD->hasInClassInitializer()))
+    return false;
+
----------------
I feel like this would be clearer as separate `if` statements; you can put 
separate comments on each.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:7084
 
+  if (FD->getParent()->isUnion() &&
+      shouldDeleteForVariantObjCPtrMember(FD, FieldType))
----------------
I believe the right check for variant-ness is `inUnion()`, not 
`FD->getParent()->isUnion()`, since the latter can miss cases with e.g. 
anonymous `struct`s.


================
Comment at: test/SemaObjCXX/arc-0x.mm:164
+    union {
+      union { // expected-note 2 {{'S1' is implicitly deleted because variant 
field '' has a non-trivial}} expected-note 4 {{'S1' is implicitly deleted 
because field '' has a deleted}}
+        id f0; // expected-note 2 {{'' is implicitly deleted because variant 
field 'f0' is an ObjC pointer}}
----------------
ahatanak wrote:
> The diagnostic message here should say the special function is deleted 
> because the anonymous union's corresponding special function is deleted, but 
> when diagnosing a deleted copy assignment operator, it says the anonymous 
> union's special function is non-trivial. I'm not sure this is a bug, but I 
> see the same diagnostic message when I compile the following non-ObjC code:
> 
> ```
> struct S0 {
>   S0(const S0 &);
>   S0 &operator=(const S0 &);
>   int *p;
> };
> 
> struct S1 {
>   union {
>     union { // copy assignment operator of 'S1' is implicitly deleted because 
> variant field '' has a non-trivial copy assignment operator
>       S0 s10;
>       int b;
>     };
>     int c;
>   };
>   ~S1();
> };
> 
> S1 *x0;
> 
> void testC1(S1 *a0) {
>   *a0 = *x0; // error: object of type 'S1' cannot be assigned because its 
> copy assignment operator is implicitly deleted
>   *a0 = static_cast<S1&&>(*x0); // error: object of type 'S1' cannot be 
> assigned because its copy assignment operator is implicitly deleted
> }
> ```
> 
> It seems that this happens because the following code in 
> `Sema::ShouldDeleteSpecialMember` is preventing the method declaration from 
> being marked as deleted:
> 
> ```
>   // For an anonymous struct or union, the copy and assignment special members
>   // will never be used, so skip the check. For an anonymous union declared at
>   // namespace scope, the constructor and destructor are used.
>   if (CSM != CXXDefaultConstructor && CSM != CXXDestructor &&
>       RD->isAnonymousStructOrUnion())
>     return false;
> ```
Well, if it's not different from the ordinary C++ treatment, I think we can 
justify just improving QoI there separately.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57438



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

Reply via email to