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

[Erich Keane via Phabricator via cfe-commits]

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