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