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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked an inline comment as done.
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:
> ahatanak wrote:
> > rjmccall wrote:
> > > ahatanak wrote:
> > > > 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)?
> > > I think "trivial" is a reasonable kind of primitive-copy semantics.  Basically, we're saying that all complete object types other than non-trivial C++ types have some sort of primitive-copy semantics, and this enum tells us what those semantics are.
> > > 
> > > DestructionKind has to deal with arrays as well, but it does so by just reporting the appropriate value for the underlying element type without mentioning that it's specifically an *array* of the type.  I think that's a reasonable design, since most places do not need to distinguish arrays from non-arrays.  IRGen does, but only when you're actually finally emitting code; prior to that (when e.g. deciding that a field is non-trivial to copy) you can just use the enum value.
> > > 
> > > You can do without getAsArrayType the same way that isDestructedType() does: the more complex operation is only necessary in order to preserve qualifiers on the element type, but that's unnecessary for this operation because the array type itself is considered to have the same qualifiers as its element type.  That is, you can ask the original type whether it's __strong/__weak-qualified without having to specifically handle array types, and once you've done all of those queries, you can use the "unsafe" operations to drill down to the element type because you no longer care about qualification.
> > So QualType::hasNonTrivialToDefaultInitializeStruct should be renamed to QualType::hasPrimitiveDefaultInitializeStruct and RecordDecl::isNonTrivialToPrimitiveDefaultInitialize to RecordDecl::isPrimitiveDefaultInitialize, for example?
> > 
> > Also, I realized that, after I made the changes that removed the NonTrivial flags from RecordDecl, CGNonTrivialStruct.cpp is the only user of enum NonTrivialCopyKind and the following methods of QualType (Sema::isValidVarArgType calls nonTrivialToDestroy, but it should call methods like hasNonTrivialToDestroyStruct that detect non-trivial C structs instead):
> > 
> > nonTrivialToDefaultInitialize
> > nonTrivialToCopy
> > nonTrivialToDestructiveMove
> > nonTrivialToDestroy
> > 
> > Maybe we should make these enums and functions local to CGNonTrivialStruct.cpp to avoid adding something that is not used outside CGNonTrivialStruct.cpp?
> I think the right name would be isNonTrivialToPrimitiveDefaultInitialize(), and like isDestructedType() it would return an enum that happens to have a zero value (and is thus false) for the trivial case.
> 
> I think we are likely to add more places that use these methods and enums.  For example, if we want to add a diagnostic that warns about memcpy'ing non-trivial struct types, that diagnostic should probably include a note about which field is non-trivial, and that will still basically want to do this same investigation.
I renamed the functions to names that start with isNonTrivialToPrimitive and removed the enumerator for "Array" from the enum too.


https://reviews.llvm.org/D41228