Page MenuHomePhabricator

implement __has_unique_object_representations
ClosedPublic

Authored by erichkeane on Oct 18 2017, 11:29 AM.

Details

Summary

A helper builtin to facilitate implementing the
std::has_unique_object_representations type trait.

Requested here: https://bugs.llvm.org/show_bug.cgi?id=34942
Also already exists in GCC and MSVC.

Diff Detail

Repository
rL LLVM

Event Timeline

erichkeane created this revision.Oct 18 2017, 11:29 AM
STL_MSFT edited edge metadata.Oct 18 2017, 3:19 PM

Looks good to me modulo comments.

lib/Parse/ParseExpr.cpp
719 ↗(On Diff #119512)

Should this be marked as MS if it's cross-vendor?

test/SemaCXX/type-traits.cpp
2403 ↗(On Diff #119512)

char is a pointer to member! What a world. (ditto below)

2420 ↗(On Diff #119512)

The use of "FP" to mean "floating point" is a little confusing given the FP typedef for Function Pointer above.

erichkeane marked 3 inline comments as done.
erichkeane added inline comments.Oct 18 2017, 3:50 PM
lib/Parse/ParseExpr.cpp
719 ↗(On Diff #119512)

Ah, right :) I didn't discover the GCC version until later, and must have forgotten this.

test/SemaCXX/type-traits.cpp
2403 ↗(On Diff #119512)

What a world indeed!

rnk added inline comments.Oct 23 2017, 5:36 PM
lib/AST/Type.cpp
2226 ↗(On Diff #119533)

What about base classes? I think that's where the padding detection is going to get wacky. =/

erichkeane added inline comments.Oct 23 2017, 6:17 PM
lib/AST/Type.cpp
2226 ↗(On Diff #119533)

Based on my reading of the RecordLayout stuff, the "getFieldOffset" should take that into account, right? It seems that 'fields' contains all fields, and thus should run through the offset of all of them, right? I'll add another test that inherits from a Padded struct to verify (as well as one that causes padding with the inheritence).

erichkeane added inline comments.Oct 24 2017, 8:03 AM
lib/AST/Type.cpp
2226 ↗(On Diff #119533)

@rnk: You're correct here I believe, I'm not properly checking the base class as I thought I was. I'll get a new patch later today.

Based on @rnk s comments, I discovered that base-class handling is more difficult than I suspected! I added a couple more tests and am properly handling base classes.

rnk added inline comments.Oct 24 2017, 12:45 PM
lib/AST/Type.cpp
2212–2213 ↗(On Diff #120106)

OK, I guess vbases don't have a unique representation. :) What about vtable slots? Should we check isDynamic?

erichkeane added inline comments.Oct 24 2017, 1:47 PM
lib/AST/Type.cpp
2212–2213 ↗(On Diff #120106)

Dynamic types are caught by "isTriviallyCopyable" pretty early in the process. I'll add a pair of tests to ensure that this remains the case.

Adding tests for excluding dynamic types.

rnk accepted this revision.Oct 24 2017, 2:18 PM

lgtm, thanks!

This revision is now accepted and ready to land.Oct 24 2017, 2:18 PM
This revision was automatically updated to reflect the committed changes.