[PATCH] D39347: Fix __has_unique_object_representations based on rsmith's input

[Erich Keane via Phabricator via cfe-commits]

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:
> > > 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.


================
Comment at: lib/AST/ASTContext.cpp:2213
+        Field->isBitField()
+            ? Field->getBitWidthValue(Context)
+            : Context.toBits(Context.getTypeSizeInChars(Field->getType()));
----------------
rsmith wrote:
> This isn't quite right; you want min(bitfield length, bit size of type) here. Eg, a `bool b : 8` or `int n : 1000` bitfield has padding bits.
I guess I'm missing something with this comment... why would a bitfield with bool b: 8 have padding?

Additionally, isn't int n : 1000 illegal, since 1000 > 32 (# bits in an int)?
Both clang and GCC reject the 2nd case.  Curiously, GCC rejects bool b: 9, but Clang seems to allow 'bool' to have ANY size.  Is that an error itself?

Finally: Should we consider ANY bool to be not a unique object representation?  It has 1 bit of data, but 8 bits of storage.  GCC's implementation of this trait accepts them, but I'm second guessing that at the moment...


================
Comment at: lib/AST/MicrosoftCXXABI.cpp:253
+
+  MPI.HasPadding = MPI.Width % MPI.Align == 0;
 
----------------
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?


https://reviews.llvm.org/D39347