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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
