This revision was automatically updated to reflect the committed changes.
Closed by commit rC319446: Fix __has_unique_object_representations
implementation (authored by erichkeane).
Repository:
rC Clang
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
erichkeane added a comment.
In https://reviews.llvm.org/D39347#940015, @rsmith wrote:
> Thanks for your patience, this turned out to be surprisingly complicated.
Of course! Thank you as well for your input, I was surprised at how
complicated this ended up being as well!
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Thanks for your patience, this turned out to be surprisingly complicated.
https://reviews.llvm.org/D39347
___
cfe-commits mailing list
erichkeane updated this revision to Diff 124863.
erichkeane added a comment.
Fix for trailing padding BITS, which obviously won't be merged.
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
lib/AST/ASTContext.cpp
lib/AST/CXXABI.h
erichkeane added inline comments.
Comment at: lib/AST/ASTContext.cpp:2200
+ Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+ CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+ if (BaseOffset != CurOffset)
rsmith wrote:
>
rsmith added inline comments.
Comment at: lib/AST/ASTContext.cpp:2200
+ Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+ CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+ if (BaseOffset != CurOffset)
erichkeane wrote:
> rsmith
erichkeane updated this revision to Diff 124859.
erichkeane added a comment.
Fixed the 'tail padding' mechanism to work correctly as suggested by @rsmith
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
lib/AST/ASTContext.cpp
erichkeane added inline comments.
Comment at: lib/AST/ASTContext.cpp:2200
+ Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+ CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+ if (BaseOffset != CurOffset)
rsmith wrote:
>
rsmith added inline comments.
Comment at: lib/AST/ASTContext.cpp:2200
+ Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+ CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+ if (BaseOffset != CurOffset)
erichkeane wrote:
> rsmith
erichkeane marked an inline comment as done.
erichkeane added inline comments.
Comment at: lib/AST/ASTContext.cpp:2200
+ Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+ CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+ if (BaseOffset !=
rsmith added inline comments.
Comment at: lib/AST/ASTContext.cpp:2200
+ Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+ CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+ if (BaseOffset != CurOffset)
On reflection, I don't think
erichkeane updated this revision to Diff 124769.
erichkeane added a comment.
Woops, missed an 'svn add' and lost the member ptr test. Back now!
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
lib/AST/ASTContext.cpp
lib/AST/CXXABI.h
erichkeane updated this revision to Diff 124765.
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
lib/AST/ASTContext.cpp
lib/AST/CXXABI.h
lib/AST/ItaniumCXXABI.cpp
lib/AST/MicrosoftCXXABI.cpp
lib/AST/Type.cpp
lib/Sema/SemaExprCXX.cpp
erichkeane added inline comments.
Comment at: lib/AST/ASTContext.cpp:2213
+Field->isBitField()
+? Field->getBitWidthValue(Context)
+: Context.toBits(Context.getTypeSizeInChars(Field->getType()));
erichkeane wrote:
> rsmith wrote:
rsmith added inline comments.
Comment at: lib/AST/ASTContext.cpp:2213
+Field->isBitField()
+? Field->getBitWidthValue(Context)
+: Context.toBits(Context.getTypeSizeInChars(Field->getType()));
erichkeane wrote:
> erichkeane wrote:
erichkeane added a comment.
Did ore looking into the bitfield shennangins. Will update this patch
tomorrow. @rsmith if you get a chance, do you perhaps have an opinion on
'bool' fields? It seems to me that a bool could either be considered an 8 bit
value, or a 1 bit value with 7 bits of
erichkeane updated this revision to Diff 124679.
erichkeane added a comment.
thanks for the comments rsmith, think this is all caught up to your comments.
Also added the test for member ptr differences.
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
erichkeane added inline comments.
Comment at: lib/AST/ASTContext.cpp:2213
+Field->isBitField()
+? Field->getBitWidthValue(Context)
+: Context.toBits(Context.getTypeSizeInChars(Field->getType()));
erichkeane wrote:
> rsmith wrote:
rsmith added inline comments.
Comment at: lib/AST/MicrosoftCXXABI.cpp:253
+
+ MPI.HasPadding = MPI.Width % MPI.Align == 0;
erichkeane wrote:
> rsmith wrote:
> > rsmith wrote:
> > > erichkeane wrote:
> > > > rsmith wrote:
> > > > > This seems to be backwards?
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:
>
rsmith added inline comments.
Comment at: lib/AST/MicrosoftCXXABI.cpp:253
+
+ MPI.HasPadding = MPI.Width % MPI.Align == 0;
rsmith wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > This seems to be backwards?
> > >
> > > Also, I'm not sure whether this is
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:
>
rsmith added a comment.
Please add tests for the member pointer cases across various different targets.
Comment at: lib/AST/ASTContext.cpp:2183-2184
+ for (const auto *Field : RD->fields()) {
+if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+ return
erichkeane updated this revision to Diff 124609.
erichkeane added a comment.
Add has padding to CXXABI getMemberPointerWidthAndAlign to correct padding
behavior here.
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
lib/AST/ASTContext.cpp
erichkeane added inline comments.
Comment at: lib/AST/ASTContext.cpp:2238-2240
+ // All other pointers are unique.
+ if (Ty->isPointerType() || Ty->isMemberPointerType())
+return true;
rsmith wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > This is not
rsmith added inline comments.
Comment at: lib/AST/ASTContext.cpp:2238-2240
+ // All other pointers are unique.
+ if (Ty->isPointerType() || Ty->isMemberPointerType())
+return true;
erichkeane wrote:
> rsmith wrote:
> > This is not correct: member pointer
erichkeane updated this revision to Diff 124490.
erichkeane marked an inline comment as done.
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
lib/AST/ASTContext.cpp
lib/AST/Type.cpp
lib/Sema/SemaExprCXX.cpp
test/SemaCXX/type-traits.cpp
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()->getDecl();
erichkeane added a comment.
Ping? This fixes a number of bugs with the previous implementation, so I'd
like to see if we can get this in.
https://reviews.llvm.org/D39347
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
erichkeane updated this revision to Diff 122897.
erichkeane added a comment.
Added a test for aligned bitfield, as @majnemer requested. Is this sufficient?
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
lib/AST/ASTContext.cpp
majnemer added a comment.
It'd be good to have tests which have alignment directives on bitfields.
https://reviews.llvm.org/D39347
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
erichkeane updated this revision to Diff 122883.
erichkeane added a comment.
Fixed for bitfields. Review by anyone greatly appreciated :)
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
lib/AST/ASTContext.cpp
lib/AST/Type.cpp
erichkeane added a comment.
Corrected version of the previous example:
struct S {
char a : 1;
char b : 2;
char c : 3;
char d : 2;
};
static_assert(sizeof(S) == 1, "size");
static_assert(__has_unique_object_representations(S), "error");
I haven't tested this patch agaainst this repro, but it
erichkeane added a comment.
Also, I should probably validate bitfields. The following SHOULD?! be unique?
struct S {
unsigned a : 1;
unsigned b : 2;
unsigned c : 3;
unsigned d : 2;
};
static_assert(__has_unique_object_representations(S), "error");
But without 'd', it should not be (tail
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;
+
erichkeane created this revision.
Corrections done for @rsmith 's comments after-commit
on cfe-commits.
https://reviews.llvm.org/D39347
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
lib/AST/ASTContext.cpp
lib/AST/Type.cpp
lib/Sema/SemaExprCXX.cpp
36 matches
Mail list logo
