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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: include/clang/AST/Type.h:1152
+    NTFK_Struct,  // non-trivial C struct.
+    NTFK_Array    // array that has non-trivial elements.
+  };
----------------
rjmccall wrote:
> We don't actually distinguish arrays in DestructionKind.  Is it important here?  You can't actually do anything with that information.
> 
> Regarding your naming question, I wonder if it would be useful to just invent a new term here, something meaning "non-trivial" but without the C++ baggage.  Maybe just "PrimitiveCopyKind", with the understanding that C++ can never do a "primitive" copy?  And for consistency with DestructionKind, maybe you should lowercase the second words.
> 
> I'm not sure how isNonTrivialToCopy is supposed to be used.  Where does the function come from?  And why is a context required when it isn't required for isDestructedType?
Enum NonTrivialCopyKind and isNonTrivialToCopy() are used just to distinguish the different types of fields in a C struct. Currently, there are four types that are handled by visitField functions in CGNonTrivialStruct.cpp:

- struct field
- array field
- __strong field
- trivial field that can be copied using memcpy

I thought using enums would make the code easier to read, but of course it's possible to remove them and instead just check the field types calling functions like getAsArrayType or getAs<RecordType>. ASTContext is passed to isNonTrivialToCopy so that it can call ASTContext::getAsArrayType to determine whether the type is an array. Maybe there is another way to detect an array that doesn't require ASTContext?

As for the naming, PrimitiveCopyKind seems find if we want to distinguish it from C++ non-trivial classes (I don't have a better name). If we are going to call non-trivial C structs primitiveCopyKind, what should we call the C structs that are trivial (those that can be copied using memcpy)?


https://reviews.llvm.org/D41228