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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

You should add a __has_feature check for this (__has_feature(objc_arc_fields)?), as well as documentation.  The documentation would go in the ARC spec.



================
Comment at: include/clang/AST/Decl.h:3575
+  /// Functions to query basic properties of non-trivial C structs.
+  bool nonTrivialToDefaultInitialize() const {
+    return NonTrivialToDefaultInitialize;
----------------
Naming style would be "isNonTrivialToDefualtInitialize" and so on for all of these.

If these aren't guaranteed to be consistent with the C++ definitions (and the copying one almost certainly isn't, since there isn't a single definition of copying in C++), I think you probably need to name these in a way that makes it clear that it's the C bit that's being queried/changed.  Perhaps adding "Primitive" to each one, e.g. isNonTrivialToPrimitiveCopy?  Richard is probably opinionated about this.


================
Comment at: lib/AST/ASTContext.cpp:5780
+  // Return true if Ty is a non-trivial C struct type that is non-trivial to
+  // destructly move or destroy.
+  if (Ty.hasNonTrivialToDestructiveMoveStruct() ||
----------------
"Return true" is just a description of the code that follows.  I would suggest something like "The block needs copy/destroy helpers if...".


================
Comment at: lib/CodeGen/CGBlocks.cpp:2244
+        CGM, byrefInfo, NonTrivialCStructByrefHelpers(valueAlignment, type));
+
   // Otherwise, if we don't have a retainable type, there's nothing to do.
----------------
You've updated the code for __block captures, but I think you missed computeBlockInfo for direct captures.


================
Comment at: lib/CodeGen/CGDecl.cpp:1421
+    destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+    cleanupKind = getARCCleanupKind();
+    break;
----------------
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.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:315
+    }
+  }
 }
----------------
Do these functions have a memcpy as a precondition?  I would expect them to do the full copy (for code size if nothing else).


================
Comment at: lib/CodeGen/CMakeLists.txt:73
   CodeGenAction.cpp
+  CodeGenCStruct.cpp
   CodeGenFunction.cpp
----------------
I think CGNonTrivialStruct.cpp would be a better name.


https://reviews.llvm.org/D41228