erichkeane added a comment.
Added all of @rsmith's comments here, I believe I have them all and have
properly responded to them.
================
Comment at: include/clang/AST/ASTContext.h:2115
+ bool hasUniqueObjectRepresentations(QualType Ty) const;
+
//===--------------------------------------------------------------------===//
----------------
Moved because @rsmith said:
I think this would make more sense as a member of ASTContext. The Type object
generally doesn't know or care about its representation.
================
Comment at: lib/AST/Type.cpp:2212
- for (const auto Base : ClassDecl->bases()) {
- if (Base.isVirtual())
- return false;
----------------
@rsmith said: Hmm, are there any cases in which we might want to guarantee the
vptr is identical across all instances?
I believe the answer to that is 'no', but if you have a case in mind, I can
change this.
================
Comment at: lib/AST/Type.cpp:2218
- // Empty base takes up 0 size.
- if (!isStructEmpty(Base.getType())) {
- if (!Base.getType().structHasUniqueObjectRepresentations(Context))
----------------
@rsmith said: This seems really fragile to me. Empty bases may or may not
occupy storage. But that's beside the point -- empty bases are just a special
case of bases with tail padding, which are not properly handled here.
I really think you should be walking the record layout rather than trying to
determine this from sizes alone.
Response:
I think I did that properly in this patch here. I re-did a bunch of the code
around this, so hopefully it is to your liking.
================
Comment at: lib/AST/Type.cpp:2233
- return StructSize == BaseSize;
- ;
-
----------------
@rsmith said: Stray semicolon?
Yep, don't know how that got in there...
================
Comment at: lib/AST/Type.cpp:2281
- if ((*this)->isArrayType())
- return Context.getBaseElementType(*this).hasUniqueObjectRepresentations(
- Context);
----------------
@rsmith said: I don't think this is correct. As a weird GNU behaviour, we can
have, say, a type with size 3 and alignment 4 (via an alignment attribute on a
typedef). An array of 1 such element has size 4, and has padding even if its
element type does not.
I think I added a test that covers that here. Is there a way to mark
non-record types with an alignment? If not, this cause problems, because the
Layout.getSize() checks in the struct handling will take care of this, since
'getSize' includes alignment.
================
Comment at: lib/AST/Type.cpp:2296
-
- // All pointers are unique, since they're just integrals.
- if ((*this)->isPointerType() || (*this)->isMemberPointerType())
----------------
@rsmith said: The second half of this comment doesn't seem right to me. They
may be represented as sequences of bits, but that doesn't make them integral.
I think you're right, I removed this part.
================
Comment at: lib/AST/Type.cpp:2303
-
- // Lambda types are not unique, so exclude them immediately.
- if (Record->isLambda())
----------------
@rsmith said: Why?
Thats a good question that I lack the answer to... I decided to remove this and
let the 'struct' handling take care of it, which seems to make sense, and even
better, seems to work.
================
Comment at: lib/AST/Type.cpp:2308
- if (Record->isUnion())
- return unionHasUniqueObjectRepresentations(Context);
- return structHasUniqueObjectRepresentations(Context);
----------------
@rsmith said: Making these members of QualType seems counterproductive. You
already have the Record here; it'd be better to make these file-static and pass
that in.
Done.
https://reviews.llvm.org/D39347
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
