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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/AST/ASTContext.cpp:2129-2130
+bool isStructEmpty(QualType Ty) {
+  assert(Ty.getTypePtr()->isStructureOrClassType() &&
+         "Must be struct or class");
+  const RecordDecl *RD = Ty.getTypePtr()->getAs<RecordType>()->getDecl();
----------------
Remove this and change the `getAs` below to `castAs`.


================
Comment at: lib/AST/ASTContext.cpp:2131
+         "Must be struct or class");
+  const RecordDecl *RD = Ty.getTypePtr()->getAs<RecordType>()->getDecl();
+
----------------
Remove the `.getTypePtr()` here; `QualType` has an overloaded `operator->` that does this for you.


================
Comment at: lib/AST/ASTContext.cpp:2136-2138
+  if (const CXXRecordDecl *ClassDecl = dyn_cast<CXXRecordDecl>(RD)) {
+    return ClassDecl->isEmpty();
+  }
----------------
No braces needed here, and use `auto *` when initializing from `dyn_cast`.


================
Comment at: lib/AST/ASTContext.cpp:2145
+                                                 const RecordDecl *RD) {
+  assert((RD->isStruct() || RD->isClass()) && "Must be struct/class type");
+  const auto &Layout = Context.getASTRecordLayout(RD);
----------------
I think you mean `!RD->isUnion()`, right? You shouldn't assert if given an `interface`.


================
Comment at: lib/AST/ASTContext.cpp:2148
+
+  CharUnits CurOffset{};
+  if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RD)) {
----------------
The LLVM coding style [does not permit](http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor) this form of initialization. Use an initializer that expresses your intent instead, such as `CharUnits::Zero()`.


================
Comment at: lib/AST/ASTContext.cpp:2150
+  if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RD)) {
+    if (ClassDecl->isDynamicClass() || ClassDecl->getNumVBases() != 0)
+      return false;
----------------
The `getNumVBases` check here is redundant; `isDynamicClass` checks that.


================
Comment at: lib/AST/ASTContext.cpp:2157-2161
+      if (!isStructEmpty(Base.getType()) &&
+          !Context.hasUniqueObjectRepresentations(Base.getType()))
+        return false;
+      Bases.push_back(Base.getType());
+    }
----------------
Maybe just omit empty classes from the `Bases` list entirely? That'd remove the need to recompute `isStructEmpty` below.


================
Comment at: lib/AST/ASTContext.cpp:2183-2184
+  for (const auto *Field : RD->fields()) {
+    if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+      return false;
+    int64_t FieldSizeInBits =
----------------
What should happen for fields of reference type?


================
Comment at: lib/AST/ASTContext.cpp:2219-2220
+  //   for unsigned integral types. -- end note ]
+  if (Ty.isNull())
+    return false;
+
----------------
If you want to do something with this case, the thing to do is to assert; it's a programming error to pass a null type here.


================
Comment at: lib/AST/ASTContext.cpp:2222
+
+  // Arrays are unique only if their element type is unique.
+  if (Ty->isArrayType())
----------------
As noted on the prior version of the patch, I don't think that's quite true. Here's an example:

```
typedef __attribute__((aligned(4))) char bad[3]; // type with size 3, align 4 (!!!)
static_assert(!__has_unique_object_representations(bad[2]));
```

Here, `ch` has unique object representations. But `bad[2]` does not, because forming the array type inserts one byte of padding after each element to keep the `ch` subobjects 4-byte aligned.

Now, this is clearly a very problematic "extension", but we're being intentionally compatible with older versions of GCC here. At some point we should start rejecting this (as recent versions of GCC do), but until we do so, we should treat it properly, which means you can't assume that array types don't insert padding between elements. Sorry this is so horrible ;-(


================
Comment at: lib/AST/ASTContext.cpp:2230-2232
+  // Functions are not unique.
+  if (Ty->isFunctionType())
+    return false;
----------------
This is redundant; functions are not trivially copyable.


================
Comment at: lib/AST/ASTContext.cpp:2238-2240
+  // All other pointers are unique.
+  if (Ty->isPointerType() || Ty->isMemberPointerType())
+    return true;
----------------
This is not correct: member pointer representations can have padding bits. In the MS ABI, a pointer to member function is a pointer followed by 0-3 ints, and if there's an odd number of ints, I expect there'll be 4 bytes of padding at the end of the representation on 64-bit targets.

I think you'll need to either add a `clang::CXXABI` entry point for determining whether a `MemberPointerType` has padding, or perhaps extend the existing `getMemberPointerWidthAndAlign` to also provide this information.


https://reviews.llvm.org/D39347