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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked 2 inline comments as done.
ahatanak added inline comments.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:315
+    }
+  }
 }
----------------
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > Do these functions have a memcpy as a precondition?  I would expect them to do the full copy (for code size if nothing else).
> > Yes, there should always be a call to memcpy before the copy/move special functions are called.  I don't think we want to fold the call to memcpy into the body of the special functions since another special function can be called from the body if the non-trivial C struct has a field whose type is a non-trivial C struct too (in which case, there will be a redundant memcpy to copy the C struct field).
> > 
> > For example, in the following code, there shouldn't be a call to memcpy to copy field "f0" of StrongOuter if there is already a memcpy that copies struct StrongOuter:
> > 
> > ```
> > typedef struct {
> >   int f0;
> >   id f1;
> > } Strong;
> > 
> > typedef struct {
> >   Strong f0;
> >   id f1;
> > } StrongOuter;
> > ```
> > 
> Well, I guess I was imagining something more C++-ish where you don't necessarily have a struct-wide memcpy, and instead you just memcpy the parts where that's profitable and otherwise do something type-specific, which would mean recursing for a struct.  Your approach is reasonable if the non-trivial copying is relatively sparse and the structure is large; on the other hand, if the non-trivial copying is dense, the memcpy itself might be mostly redundant.  And it does mean a bigger code-size hit in the original place that kicks off the copy.
> 
> IIRC we do already have some code in copy-constructor emission that's capable of emitting sequences of field copies with a memcpy, if you wanted to try the C++ style, you could probably take advantage of that pretty easily.  But I won't hold up the patch if you think the memcpy precondition is the right way to go.
I made changes so that either a load/store pair or a call to memcpy is emitted in the copy/move special functions to copy fields that are not non-trivial.


================
Comment at: lib/CodeGen/CodeGenCStruct.cpp:27
+  FK_Array   // array that has non-trivial elements.
+};
+}
----------------
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > 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.
> > I'm not sure if I understood your point here. Do you mean there should be DestructionKinds for arrays or structs too or merge these enums into DestructionKind?
> I was suggesting there should be equivalents to DestructionKind for some of the other operations, like a NonTrivialCopyKind, and queries on QualType analogous to isDestructedType(), e.g. isNonTrivialToCopy().  That way you can directly put this stuff in the AST, and then here you can just walk the struct fields and switch on the enum, at which point I think you don't need FieldKind at all.  Remember that some types are non-trivial in only a subset of these dimensions.
I added enum NonTrivialCopyKind and function isNonTrivialToCopy to QualType and removed FieldKind from CGNonTrivialStruct.cpp. Since there are special functions other than copy constructor/assignment operator, should the enum's name be NonTrivialStructKind or something instead of NonTrivialCopyKind?


https://reviews.llvm.org/D41228