rjmccall 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;
ahatanak wrote:
> 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 
> separately.
Well, we already have a type that would have a trivial deinitializing move: ARC 
`__strong` pointers.  What we don't have is anything in IRGen that currently 
would take advantage of that fact.  Anyway, I agree that we can wait to start 
tracking this until we add such code to the compiler.

Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:193
+    TrivialFieldIsVolatile |= FT.isVolatileQualified();
+    if (Start == End)
ahatanak wrote:
> 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?
Oh, I see, there's specific logic to not flag the struct trivial if a field is 
merely VolatileTrivial.  Okay, that works for me; we can think about the right 
thing to do in a follow-up commit.  It probably needs to be raised as an RFC on 

The C++ standard has explicit language about triviality that says that volatile 
fields don't make defaulted copy-constructors non-trivial.  Whatever we do here 
can't break that.  But that doesn't necessarily imply that we should continue 
to use a simple memcpy to copy the struct; we could still use the C logic to 
copy "trivial" C++ structs with volatile members.

If we do try to apply this rule to C++, you will need to propagate these bits 
appropriately for C++ types, including from bases.

Comment at: lib/Sema/SemaDecl.cpp:15424
+      if (FT.isNonTrivialToPrimitiveCopy() != QualType::PCK_Trivial &&
+          FT.isNonTrivialToPrimitiveCopy() != QualType::PCK_VolatileTrivial)
+        Record->setNonTrivialToPrimitiveCopy();
Please compute isNonTrivialToPrimitiveCopy once here, it's not trivial.

...arguably we should figure out a way to do all these type checks once and 
then set the bits from that.  We can do that as a separate patch, though.


cfe-commits mailing list

Reply via email to