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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/AST/Decl.h:3529
+  /// Basic properties of non-trivial C structs.
+  bool HasStrongObjCPointer : 1;
+
----------------
Is it interesting to track all the individual reasons that a struct is non-trivial at the struct level, as opposed to (like we do with CXXDefinitionData) just tracking the four aggregate properties you've described below?


================
Comment at: include/clang/AST/Type.h:1098
+  /// and return the kind.
+  PrimitiveCopyKind isNonTrivialToPrimitiveDefaultInitialize() const;
+
----------------
I think this one should probably get its own enum.  It's not too hard to imagine types (maybe relative references, or something else that's address-sensitive?) that require non-trivial copy semantics but isn't non-trivial to default-initialize.


================
Comment at: include/clang/AST/Type.h:1108
+  /// move and return the kind.
+  PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;
+
----------------
You need to define what you mean by "destructive move":

- A C++-style move leaves the source in a still-initialized state, just potentially with a different value (such as nil for a __strong reference).  This is what we would do when e.g. loading from an r-value reference in C++.  It's also what we have to do when moving a __block variable to the heap, since there may still be pointers to the stack copy of the variable, and since the compiler will unconditionally attempt to destroy that stack copy when it goes out of scope.  Since the source still needs to be destroyed, a non-trivially-copyable type will probably always have to do non-trivial work to do this.  Because of that, a function for this query could reasonably just return PrimitiveCopyKind.

- A truly primitive destructive move is something that actually leaves the source uninitialized.  This is generally only possible in narrow circumstances where the compiler can guarantee that the previous value is never again going to be referenced, including to destroy it.  I don't know if you have a reason to track this at all (i.e. whether there are copies you want to perform with a destructive move in IRGen), but if you do, you should use a different return type, because many types that are non-trivial to "C++-style move" are trivial to "destructively move": for example, __strong references are trivial to destructively move.


================
Comment at: include/clang/AST/Type.h:1113
+  /// the kind.
+  PrimitiveCopyKind isNonTrivialToPrimitiveDestroy() const;
+
----------------
Is there a reason we need this in addition to isDestructedType()?  It seems to me that it's exactly the same except ruling out the possibility of a C++ destructor.


================
Comment at: include/clang/AST/Type.h:1137
+    return isNonTrivialToPrimitiveDestroy() == PCK_Struct;
+  }
+
----------------
Are these four queries really important enough to provide API for them on QualType?


================
Comment at: include/clang/AST/Type.h:1148
+    DK_objc_weak_lifetime,
+    DK_c_struct_strong_field
   };
----------------
I don't think you want to refer to the fact that the C struct specifically contains a __strong field here.  As we add more reasons, would we create a new enumerator for each?  What if a struct is non-trivial for multiple reasons?  Just say that it's a non-trivial C struct.


================
Comment at: lib/AST/ASTContext.cpp:5778
+  if (Ty.isNonTrivialToPrimitiveDestructiveMoveStruct() ||
+      Ty.isNonTrivialToPrimitiveDestroyStruct())
+    return true;
----------------
I think you should use the non-struct-specific checks here.  In addition to being cleaner, it would also let you completely short-circuit the rest of this function in ARC mode.  In non-ARC mode, you do still have to check lifetime (only to see if it's __unsafe_unretained; the __strong and __weak cases would be unreachable), and you need the type-specific special cases.


https://reviews.llvm.org/D41228