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

[Akira Hatanaka via Phabricator via cfe-commits]

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