rsmith added inline comments.
================ Comment at: clang/lib/AST/Type.cpp:2500 + return BaseElementType.isTriviallyCopyableType(Context) && + !BaseElementType.isDestructedType(); + } ---------------- devin.jeanpierre wrote: > rsmith wrote: > > You can simplify this a little by dropping this `isDestructedType()` check; > > trivially copyable implies trivially destructible. > > > > It would be a little more precise to check for > > `!isNonTrivialToPrimitiveDestructiveMove() && !isDestructedType()` here; > > that'd treat `__autoreleasing` pointers as trivially-relocatable, but not > > `__strong` pointers (because "destructive move" there actually means > > "non-destructive move" and needs to leave the object in a valid but > > unspecified state, which means nulling out a moved-from `__strong` > > pointer). I think getting this "fully" right would require a bit more > > plumbing to properly handle the case of a `struct` containing a `__strong` > > pointer (which is trivially relocatable but not trivially anything else). > Oooh. `isNonTrivialToPrimitiveDestructiveMove` is perfect. > > Actually, a struct containing a `__strong` pointer is already handled -- > Clang > [already](https://github.com/llvm/llvm-project/blob/603c1a62f8595f70c3e96ecbec8976d411c0cc08/clang/lib/AST/DeclCXX.cpp#L1037-L1059) > ensures they're passed in registers, and does all the appropriate logic. > It's just `__strong` pointers themselves that I'd need to handle the same way > here. Unfortunately, I really, *really* don't understand the source code > dealing with `__strong` pointers in structs. (In particular, I don't > understand the difference between GC::Strong and ObjCLifetime::OCL_Strong, > plus I'm uncomfy with the global language opts.) > > It's tempting to use the return value of > `isNonTrivialToPrimitiveDestructiveMove` and check for `Strong` -- but now > that's some *third* thing, and not mirroring the logic of when it's in a > struct as a data member. The existing handling only covers Objective-C++ and not (non-C++) Objective-C (the C and C++ codepaths for computing struct properties are entirely different, because the C++ side of things is primarily working out properties of special member functions). It looks like your test coverage doesn't cover the non-C++ side of this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/ https://reviews.llvm.org/D114732 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
