[cfe-commits] [PATCH] C++0x strongly typed enums
Daniel Wallin
daniel at boostpro.com
Fri Oct 8 05:22:43 PDT 2010
On Fri, Oct 1, 2010 at 5:15 PM, Douglas Gregor <dgregor at apple.com> wrote:
> Hi Daniel,
Thank you for your comments!
> On Sep 30, 2010, at 1:05 PM, Daniel Wallin wrote:
>> Attached is a patch that implements partial support for strongly typed
>> enums. It lacks support for opaque enums, and there are at least a few
>> cases where a scoped enum is accepted where it shouldn't be. Comments
>> welcome!
>
>
> @@ -2119,6 +2131,23 @@ public:
> NumNegativeBits = Num;
> }
>
> + /// \brief Returns true if this is a C++0x scoped enumeration.
> + bool isScoped() const {
> + return IsScoped;
> + }
> + void setScoped(bool Scoped) {
> + IsScoped = Scoped;
> + }
> +
> + /// \brief Returns true if this is a C++0x enumeration with fixed underlying
> + /// type.
> + bool isFixed() const {
> + return IsFixed;
> + }
> + void setFixed(bool Fixed) {
> + IsFixed = Fixed;
> + }
> +
>
> I don't think we actually need the setScoped/setFixed mutators. The only client is the ASTReader, which should just be granted friendship.
Agreed.
> --- a/include/clang/Basic/DiagnosticParseKinds.td
> +++ b/include/clang/Basic/DiagnosticParseKinds.td
> @@ -378,6 +378,9 @@ def err_friend_decl_defines_class : Error<
> def warn_deleted_function_accepted_as_extension: ExtWarn<
> "deleted function definition accepted as a C++0x extension">, InGroup<CXX0x>;
>
> +def err_scoped_enum_missing_identifier : Error<
> + "scoped enumeration requires an identifier">;
>
> Nit-pick alert: "scoped enumeration requires a name" would feel a little less pedantic and more friendly, to me.
Agreed.
> --- a/include/clang/Basic/DiagnosticSemaKinds.td
> +++ b/include/clang/Basic/DiagnosticSemaKinds.td
> @@ -820,6 +820,10 @@ def err_final_function_overridden : Error<
> "declaration of %0 overrides a 'final' function">;
> def err_final_base : Error<
> "derivation from 'final' %0">;
> +
> +// C++0x scoped enumerations
> +def err_enum_invalid_underlying : Error<
> + "%0 is an invalid underlying type">;
>
> I'd prefer that this diagnostic said specifically what's wrong with the type: either it's not an integral type (the common case), or it's a wide-character type. Something like:
>
> +def err_enum_invalid_underlying : Error<
> + "%select{non-integral|wide character}0 type %1 is an invalid underlying type">;
Agreed.
> --- a/include/clang/Sema/Sema.h
> +++ b/include/clang/Sema/Sema.h
> @@ -785,7 +785,8 @@ public:
> IdentifierInfo *Name, SourceLocation NameLoc,
> AttributeList *Attr, AccessSpecifier AS,
> MultiTemplateParamsArg TemplateParameterLists,
> - bool &OwnedDecl, bool &IsDependent);
> + bool &OwnedDecl, bool &IsDependent, bool ScopedEnum,
> + ParsedType BaseType, SourceLocation BaseTyLoc);
>
> We shouldn't actually need BaseTyLoc, since the BaseType will contain a source range covering the entire type. It *might* be handy to have the location of the ':', however.
Yes, good point. I'm leaving out the location of ':' for now, since it
isn't clear that it is needed for anything.
> diff --git a/lib/AST/Type.cpp b/lib/AST/Type.cpp
> index 8e6aa23..43ce7ee 100644
> --- a/lib/AST/Type.cpp
> +++ b/lib/AST/Type.cpp
> @@ -470,6 +470,20 @@ bool Type::isIntegralOrEnumerationType() const {
> return false;
> }
>
> +bool Type::isIntegralOrUnscopedEnumerationType() const {
> + if (const BuiltinType *BT = dyn_cast<BuiltinType>(CanonicalType))
> + return BT->getKind() >= BuiltinType::Bool &&
> + BT->getKind() <= BuiltinType::Int128;
> +
> + // Check for a complete enum type; incomplete enum types are not properly an
> + // enumeration type in the sense required here.
> + if (const TagType *TT = dyn_cast<TagType>(CanonicalType))
> + if (const EnumDecl *ED = dyn_cast<EnumDecl>(TT->getDecl()))
> + return ED->isDefinition() && !ED->isScoped();
> +
> + return false;
> +}
>
> Perhaps it isn't modeled yet, but isn't a forward-declared enumeration type in C++0x (which has a fixed underlying type) considered complete?
Yes, I've adressed this in this new patch.
> + if (!Name && IsScopedEnum) {
> + // C++0x 7.2p2: The optional identifier shall not be omitted in the
> + // declaration of a scoped enumeration.
> + Diag(Tok, diag::err_scoped_enum_missing_identifier);
> + }
>
> From a recovery standpoint, it's probably safest to set IsScopedEnum = false; in here. That way, we'll maintain the AST invariant that scoped enumerations always have names.
OK, doing that.
> + ParsedType BaseType;
> + SourceLocation BaseTyLoc;
> +
> + if (getLang().CPlusPlus0x && Tok.is(tok::colon)) {
> + ConsumeToken();
> + SourceRange Range;
> + TypeResult BaseTy(ParseTypeName(&Range));
> + BaseType = BaseTy.get();
> + BaseTyLoc = Range.getBegin();
> + }
>
> If parsing the type fails, we should probably recover by setting the underlying type to "int", rather than not having an underlying type. For one, this means that we'll be able to consider such a type as "complete" for the rest of the translation unit.
OK, doing that as well.
> --- a/lib/Sema/SemaDecl.cpp
> +++ b/lib/Sema/SemaDecl.cpp
> @@ -5757,14 +5758,43 @@ CreateNewDecl:
> TagDecl *New;
>
> if (Kind == TTK_Enum) {
> +
> + QualType UnderlyingType;
> + bool IsFixed = false;
> +
> + if (!BaseType && ScopedEnum) {
> + // No underlying type explicitly specified, default to int.
> + IsFixed = true;
> + UnderlyingType = Context.IntTy;
> + }
> + else if (BaseType) {
> + TypeSourceInfo *TInfo;
> + UnderlyingType = GetTypeFromParser(BaseType, &TInfo);
> + UnderlyingType = UnderlyingType.getUnqualifiedType();
> + IsFixed = true;
> +
> + if (!UnderlyingType->isIntegralType(Context) ||
> + UnderlyingType->isWideCharType()) {
>
> What about char16_t/char32_t?
Turns out this wasn't in the working paper at all. I believe it was in
the original paper, but not in the actual proposed wording. I've
removed the wchar_t test.
> @@ -6817,7 +6851,12 @@ EnumConstantDecl *Sema::CheckEnumConstant(EnumDecl *Enum,
> // is the type of its initializing value:
> // - If an initializer is specified for an enumerator, the
> // initializing value has the same type as the expression.
> - EltTy = Val->getType();
> + if (Enum->isFixed()) {
> + EltTy = Enum->getIntegerType();
> + }
> + else {
> + EltTy = Val->getType();
> + }
> }
> }
> }
> @@ -6834,7 +6873,12 @@ EnumConstantDecl *Sema::CheckEnumConstant(EnumDecl *Enum,
> //
> // GCC uses 'int' for its unspecified integral type, as does
> // C99 6.7.2.2p3.
> - EltTy = Context.IntTy;
> + if (Enum->isFixed()) {
> + EltTy = Enum->getIntegerType();
> + }
> + else {
> + EltTy = Context.IntTy;
> + }
> } else {
> // Assign the last value + 1.
> EnumVal = LastEnumConst->getInitVal();
> @@ -6854,7 +6898,7 @@ EnumConstantDecl *Sema::CheckEnumConstant(EnumDecl *Enum,
> // sufficient to contain the incremented value. If no such type
> // exists, the program is ill-formed.
> QualType T = getNextLargerIntegralType(Context, EltTy);
> - if (T.isNull()) {
> + if (T.isNull() || Enum->isFixed()) {
> // There is no integral type larger enough to represent this
> // value. Complain, then allow the value to wrap around.
> EnumVal = LastEnumConst->getInitVal();
>
> I don't think this is quite enough. When there is a fixed underlying type and we have a value explicitly given, we should check that the value is representable in that fixed underlying type, and convert the expression to that fixed underlying type (this is [dcl.enum]p5).
Yep, I've tried to do this in the new patch. I could be missing
something though. This also adds support for forward declaration of
enums with fixed underlying type.
--
Daniel Wallin
BoostPro Computing
http://www.boostpro.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: strong-enums-2.patch
Type: application/octet-stream
Size: 47464 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20101008/c3347cfa/attachment.obj>
More information about the cfe-commits
mailing list