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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
