[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: include/clang/AST/Type.h:1108
+    PCK_ARCStrong,  // objc strong pointer.
+    PCK_Struct   // non-trivial C struct.
+  };
----------------
rjmccall wrote:
> These should all be /// documentation comments, and they mostly shouldn't talk about fields since this is a query on QualType, not FieldDecl.  I would suggest something like:
> 
> for Trivial - The type does not fall into any of the following categories.  Note that this case is zero-valued so that values of this enum can be used as a boolean condition for non-triviality.
> 
> for VolatileTrivial - The type would be trivial except that it is volatile-qualified.  Types that fall into one of the other non-trivial cases may additionally be volatile-qualified.
> 
> for ARCStrong - The type is an Objective-C retainable pointer type that is qualified with the ARC __strong qualifier.
> 
> for Struct - The type is a struct containing a field whose type is not PCK_Trivial.  Note that a C++ struct type does not necessarily match this; C++ copying semantics are too complex to express here, in part because they depend on the exact constructor or assignment operator that is chosen by overload resolution to do the copy.
Thanks, I copied your comments verbatim except the comment on PCK_Struct: types that are PCK_Struct have a field that is neither PCK_Trivial nor PCK_VolatileTrivial. We can use the original comment once we start distinguishing PCK_Trivial structs that have volatile fields from those that don't. 


================
Comment at: lib/CodeGen/CGBlocks.cpp:1779
+    break;
   }
 
----------------
rjmccall wrote:
> This entire switch can be within an `#ifndef NDEBUG` if you really feel it's important to assert.
It's probably not important to assert here, so I removed the switch.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1792
+    return std::make_pair(BlockCaptureEntityKind::BlockObject,
+                          getBlockFieldFlagsForObjCObjectPointer(CI, T));
 
----------------
rjmccall wrote:
> I think you should leave the byref case of that function here, but you can make it local to this if-clause.
I also simplified the QualType::DK_none case below.


================
Comment at: lib/CodeGen/CGDecl.cpp:1421
+    destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+    cleanupKind = getARCCleanupKind();
+    break;
----------------
rjmccall wrote:
> rjmccall wrote:
> > rjmccall wrote:
> > > ahatanak wrote:
> > > > rjmccall wrote:
> > > > > This can only be getARCCleanupKind() if it's non-trivial to destroy solely because of __strong members.  Since the setup here is significantly more general than ARC, I think you need to default to the more-correct behavior; if you want to add a special-case check for only __strong members, you ought to do that explicitly.
> > > > I added a DestructionKind (DK_c_struct_strong_field) that is used just for structs that have strong fields (but doesn't have weak fields).
> > > Sure, that's a simple enough way to do it, and I think it's fairly warranted.
> > Okay, since you don't have a DK_c_struct_strong_field anymore, you either need to stop using getARCCleanupKind() or do the test for just __strong fields.
> Ping about this again.
Sorry I missed this one.


https://reviews.llvm.org/D41228