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
