ahatanak added inline comments.

Comment at: include/clang/AST/Type.h:1121
+  /// after it is moved, as opposed to a truely destructive move in which the
+  /// source object is placed in an uninitialized state.
+  PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;
rjmccall wrote:
> "truly"
> Hmm.  Now that I'm thinking more about it, I'm not sure there's any point in 
> tracking non-triviality of a C++-style destructive move separately from the 
> non-triviality of a copy.  It's hard to imagine that there would ever be a 
> non-C++ type that primitively has non-trivial copies but trivial C++-style 
> moves or vice-versa.  Type-based destructors imply that the type represents 
> some kind of resource, and a C++-style move will always be non-trivial for 
> resource types because ownership of the resource needs to be given up by the 
> old location.  Otherwise, a type might be non-trivial to copy but not destroy 
> because there's something special about how it's stored (like volatility), 
> but it's hard to imagine what could possibly cause it to be non-trivial to 
> destroy but not copy.
> If we were tracking non-triviality of an *unsafe* destructive move, one that 
> leaves the source in an uninitialized state, that's quite different.
> I think there are three reasonable options here:
> - Ignore the argument I just made about the types that we're *likely* to care 
> about modeling and generalize your tracking to also distinguish construction 
> from assignment.  In such an environment, I think you can absolutely make an 
> argument that it's still interesting to track C++-style moves separately from 
> copies.
> - Drop the tracking of destructive moves completely.  If you want to keep the 
> method around, find, but it can just call `isNonTrivialToPrimitiveCopy()`.
> - Change the tracking of *destructive* moves to instead track 
> *deinitializing* moves.  The implementation would stop considering `__strong` 
> types to be non-trivial to move.
> But as things stand today, I do not see any point in separately tracking 
> triviality of C++-style destructive moves.
The second option seems most reasonable to me. We can always make changes if 
someone comes up with a type that requires tracking destructive moves 

Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:193
+    TrivialFieldIsVolatile |= FT.isVolatileQualified();
+    if (Start == End)
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > I feel like maybe volatile fields should be individually copied instead 
> > > of being aggregated into a multi-field memcpy.  This is a more natural 
> > > interpretation of the C volatile rules than we currently do.  In fact, 
> > > arguably we should really add a PrimitiveCopyKind enumerator for volatile 
> > > fields (that are otherwise trivially-copyable) and force all copies of 
> > > structs with volatile fields into this path precisely so that we can make 
> > > a point of copying the volatile fields this way.  (Obviously that part is 
> > > not something that's your responsibility to do.)
> > > 
> > > To get that right with bit-fields, you'll need to propagate the actual 
> > > FieldDecl down.  On the plus side, that should let you use 
> > > EmitLValueForField to do the field projection in the common case.
> > I added method visitVolatileTrivial that copies volatile fields 
> > individually. Please see test case test_copy_constructor_Bitfield1 in 
> > test/CodeGenObjC/strong-in-c-struct.m.
> Okay, great!  I like the name.
> Does this mean we're now copying all structs that contain volatile fields 
> with one of these helper functions?  If so, please add a C test case testing 
> just that.  Also, you should retitle this review and stress that we're 
> changing how *all* non-trivial types are copied, and that that includes both 
> volatile and ARC-qualified fields.
No, the current patch doesn't copy volatile fields of a struct individually 
unless the struct is a non-trivial type (which means its primitive copy kind is 
PCK_Struct). I'll look into today how I can force structs with volatile fields 
that are not non-trivial to be copied using the helper functions.

It seems like we would need a boolean flag in RecordDecl that tracks the 
presence of volatile fields in the struct or one of its subobjects. I assume we 
want to copy volatile fields individually in C++ too, in which case the flag 
needs to be set in both C and C++ mode. Is that right?


cfe-commits mailing list

Reply via email to