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