Review Request: Plain Old Data (POD) property of classes, and type traits basics

Hi,

I've implemented the POD property of classes, and (in order to test it)
a basic infrastructure for GCC's type traits extensions, plus a concrete
implementation of the __is_pod trait.

However, I'm not quite sure about all aspects of the patch, so I'm
posting it for review first. In particular, I have an enum called
UnaryTypeTrait that represents the various type trait keywords. Since it
is needed in the parser, the sema and the AST, I saw no other choice but
to make it part of Basic. Is this the right thing to do?

Patch is attached.

Sebastian

pod.patch (39.2 KB)

Hi Sebastian,

I've implemented the POD property of classes, and (in order to test it)
a basic infrastructure for GCC's type traits extensions, plus a concrete
implementation of the __is_pod trait.

Cool.

However, I'm not quite sure about all aspects of the patch, so I'm
posting it for review first. In particular, I have an enum called
UnaryTypeTrait that represents the various type trait keywords. Since it
is needed in the parser, the sema and the AST, I saw no other choice but
to make it part of Basic. Is this the right thing to do?

Yes, I think so. AccessSpecifier, which currently lives in Parse, should also be moved back to Basic (but that's my problem, not yours).

+/// UnaryTypeTraitExpr - A GCC or MS unary type trait, as used in the
+/// implementation of TR1/C++0x type trait templates.

Please add an example to the documentation for this class.

+ // FIXME: Some of the type traits have requirements. Interestingly, only the
+ // __is_base_of requirement is explicitly stated to be diagnosed. Indeed,
+ // G++ accepts __is_pod(Incomplete) without complaints, and claims that the
+ // type is indeed a POD.

Interesting. The documentation for __is_pod says:

  Requires: type shall be a complete type, an array type of unknown bound, or is a void type.

So GCC should be diagnosing this kind of error. We should report the problem to them. However, we can address this FIXME later. The point of the patch is PODs, right? :slight_smile:

In Type::isPODType, could you add a FIXME for pointer-to-member types? (I almost want to just add basic pointer-to-member support now, since we seem to have lots of FIXMEs for them).

Regarding the FIXME at the end of CXXRecordDecl::hasConstCopyAssignment, I think we should assert that the CXXRecordDecl itself is marked "invalid". We shouldn't get here in any well-formed program.

This patch looks really good to me. Thanks!

  - Doug

Douglas Gregor wrote:

Hi Sebastian,

However, I'm not quite sure about all aspects of the patch, so I'm
posting it for review first. In particular, I have an enum called
UnaryTypeTrait that represents the various type trait keywords. Since it
is needed in the parser, the sema and the AST, I saw no other choice but
to make it part of Basic. Is this the right thing to do?

Yes, I think so. AccessSpecifier, which currently lives in Parse,
should also be moved back to Basic (but that's my problem, not yours).

OK.

+/// UnaryTypeTraitExpr - A GCC or MS unary type trait, as used in the
+/// implementation of TR1/C++0x type trait templates.

Please add an example to the documentation for this class.

Done.

+ // FIXME: Some of the type traits have requirements. Interestingly,
only the
+ // __is_base_of requirement is explicitly stated to be diagnosed.
Indeed,
+ // G++ accepts __is_pod(Incomplete) without complaints, and claims
that the
+ // type is indeed a POD.

Interesting. The documentation for __is_pod says:

    Requires: type shall be a complete type, an array type of unknown
bound, or is a void type.

The candidate draft says the same thing, word for word, about
std::is_pod<T>.

So GCC should be diagnosing this kind of error. We should report the
problem to them. However, we can address this FIXME later. The point
of the patch is PODs, right? :slight_smile:

In Type::isPODType, could you add a FIXME for pointer-to-member types?
(I almost want to just add basic pointer-to-member support now, since
we seem to have lots of FIXMEs for them).

Done. The FIXME, not member pointers. :frowning:

Regarding the FIXME at the end of
CXXRecordDecl::hasConstCopyAssignment, I think we should assert that
the CXXRecordDecl itself is marked "invalid". We shouldn't get here in
any well-formed program.

OK.

This patch looks really good to me. Thanks!

Thanks for the review.

Sebastian