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



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

Reply via email to