erichkeane added inline comments.
================ Comment at: lib/AST/ASTContext.cpp:2183-2184 + for (const auto *Field : RD->fields()) { + if (!Context.hasUniqueObjectRepresentations(Field->getType())) + return false; + int64_t FieldSizeInBits = ---------------- rsmith wrote: > erichkeane wrote: > > rsmith wrote: > > > erichkeane wrote: > > > > rsmith wrote: > > > > > What should happen for fields of reference type? > > > > References are not trivially copyable, so they will prevent the struct > > > > from having a unique object representation. > > > That sounds like the wrong behavior to me. If two structs have references > > > that bind to the same object, then they have the same object > > > representation, so the struct does have unique object representations. > > I didn't think of it that way... I WILL note that GCC rejects references in > > their implementation, but that could be a bug on their part. > The wording you quote below is defective in this regard (it doesn't discuss > what "same value" means for reference members) -- that's at least partly my > wording, sorry about that! But at least my intent when we were working on the > rules was that "same value" would have the obvious structurally-recursive > meaning, which for references means we're considering whether they have the > same referent. > > So I think references, like pointers, should always be considered to have > unique object representations when considered as members of objects of class > type. (But `__has_unique_object_representations(T&)` should still return > `false` because `T&` is not a trivially-copyable type, even though a class > containing a `T&` might be.) That makes sense to me, I can change that. ================ Comment at: lib/AST/MicrosoftCXXABI.cpp:253 + + MPI.HasPadding = MPI.Width % MPI.Align == 0; ---------------- rsmith wrote: > rsmith wrote: > > erichkeane wrote: > > > rsmith wrote: > > > > This seems to be backwards? > > > > > > > > Also, I'm not sure whether this is correct. In the strange case where > > > > `Width` is not a multiple of `Align` (because we don't round up the > > > > width), there is no padding. We should only set `HasPadding` to `true` > > > > in the `alignTo` case below. > > > I think you're right about it being backwards. > > > > > > However, doesn't the struct with a single Ptr and either 1 or 3 Ints have > > > tail padding as well? I do believe you're right about the alignTo > > > situation below, but only if Width changes, right? > > I think the idea is that a struct with one pointer and an odd number of > > ints, on 32-bit Windows, will have no padding per se, but its size will > > simply not be a multiple of its alignment. (So struct layout can put things > > in the four bytes after it, but will insert padding before it to place it > > at an 8 byte boundary.) > See example here: https://godbolt.org/g/Nr8C2L Well... oh dear. That would mean that: auto p = &A::N; static_assert(!has_unique_object_representations_v<decltype(p)>); // not unique, since it has padding, right? struct S { decltype(p) s; }; static_assert(!has_unique_object_representations_v<S>); // also not unique, since 's' has padding. struct S2 { decltype(p) s; int a; }; static_assert(has_unique_object_representations_v<S2>); // Actually unique again, since the padding is filled. Do I have this right? https://reviews.llvm.org/D39347 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits