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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I accidentally hit 'send' too early on my review, so here's part two.  I still haven't fully reviewed the new IRGen file.



================
Comment at: lib/CodeGen/CodeGenCStruct.cpp:27
+  FK_Array   // array that has non-trivial elements.
+};
+}
----------------
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.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1144
+    // indirectly, the callee is responsible for destructing the argument.
+    if (Ty.hasNonTrivialToDestroyStruct()) {
+      AutoVarEmission Emission(*VD);
----------------
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.


================
Comment at: lib/CodeGen/CodeGenFunction.h:3347
+                                  QualType QT, bool IsVolatile);
+  void callDestructor(Address DstPtr, QualType QT, bool IsVolatile);
+
----------------
These methods should *definitely* be somehow namespaced for your new feature.


https://reviews.llvm.org/D41228