ahatanak marked 9 inline comments as done. ahatanak added a comment. Address review comments.
================ Comment at: lib/CodeGen/CGDecl.cpp:1421 + destroyer = CodeGenFunction::destroyNonTrivialCStruct; + cleanupKind = getARCCleanupKind(); + break; ---------------- 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). ================ Comment at: lib/CodeGen/CGExprAgg.cpp:315 + } + } } ---------------- rjmccall wrote: > Do these functions have a memcpy as a precondition? I would expect them to > do the full copy (for code size if nothing else). Yes, there should always be a call to memcpy before the copy/move special functions are called. I don't think we want to fold the call to memcpy into the body of the special functions since another special function can be called from the body if the non-trivial C struct has a field whose type is a non-trivial C struct too (in which case, there will be a redundant memcpy to copy the C struct field). For example, in the following code, there shouldn't be a call to memcpy to copy field "f0" of StrongOuter if there is already a memcpy that copies struct StrongOuter: ``` typedef struct { int f0; id f1; } Strong; typedef struct { Strong f0; id f1; } StrongOuter; ``` ================ Comment at: lib/CodeGen/CodeGenCStruct.cpp:27 + FK_Array // array that has non-trivial elements. +}; +} ---------------- rjmccall wrote: > There's no need to put this in an anonymous namespace. > > This enum makes me wonder if it would make more sense to create equivalents > to QualType::DestructionKind for each of these operations and all of the > different primitive types. That would let you e.g. implement your > only-strong-members check above much more easily, and it would also make it > simpler to guarantee that all the places that need to exhaustively enumerate > the reasons why something might need special initialization/copying logic > don't miss an important new case. I'm not sure if I understood your point here. Do you mean there should be DestructionKinds for arrays or structs too or merge these enums into DestructionKind? ================ Comment at: lib/CodeGen/CodeGenFunction.cpp:1144 + // indirectly, the callee is responsible for destructing the argument. + if (Ty.hasNonTrivialToDestroyStruct()) { + AutoVarEmission Emission(*VD); ---------------- rjmccall wrote: > You're not actually checking the "is not passed indirectly" part of this, but > I think that's okay, because I don't think we actually want the ownership > aspects of the ABI to vary based on how the argument is passed. So you > should just strike that part from the comment. > > Also, this should really be done in EmitParmDecl. To make ARC and ARC++ compatible, I think we'll have to change the ABI of C++ functions that are passed structs containing weak fields if we want to destruct non-trivial C arguments destructed in the callee. I guess that's OK since we have to break the ABI for structs containing strong fields too. ================ Comment at: lib/CodeGen/CodeGenFunction.h:3347 + QualType QT, bool IsVolatile); + void callDestructor(Address DstPtr, QualType QT, bool IsVolatile); + ---------------- rjmccall wrote: > These methods should *definitely* be somehow namespaced for your new feature. I renamed these methods to make it clear that they are calling special functions for C structs. Is that what you mean by "namespaced"? https://reviews.llvm.org/D41228 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits