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 separately. ================ 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? https://reviews.llvm.org/D41228 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
