Support enums with a fixed underlying type in all language modes

[erik.pilkington]

Summary

A significant number of internal C users have been complaining about the lack of support for fixed enums. We already support this in Objective-C mode, as well as in C mode with -fms-extensions. Supporting this in C makes us more consistent, and provides a useful feature to C users!

If there is some doubt as to whether we want this, I can start a thread on cfe-dev for the wider audience.

rdar://43831380

Thanks for taking a look!

[aaron.ballman]

I really like this idea in general, thank you for proposing this patch!

clang/include/clang/Basic/Features.def

90 ↗

(On Diff #166397)

Is this really supported in ObjC1, or is there no longer a distinction to be made there?

233 ↗

(On Diff #166397)

Are we sure we want to make this decision for things like OpenCL, Cuda, etc?

[aaron.ballman]

Whoops, that acceptance was accidental. Pretending I want changes just to make it clear in Phab. :-)

[jfb]

I think we should consider proposing this to the C committee. @aaron.ballman I can help Erik write the paper, would you be able to present it? Too tight for the upcoming meeting, but I'm guessing we have *plenty* of time for ++C17.

[aaron.ballman]

In D52339#1242086, @jfb wrote:

I think we should consider proposing this to the C committee. @aaron.ballman I can help Erik write the paper, would you be able to present it? Too tight for the upcoming meeting, but I'm guessing we have *plenty* of time for ++C17.

It's already been proposed to WG14 and is currently on the SD-3 list of features to consider for C2x. See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2008.pdf. I know Clive and am happy to point him towards this patch (when it lands) as demonstration of industry desire for the feature, in case he needs to provide updated papers.

[jfb]

In D52339#1242103, @aaron.ballman wrote:

In D52339#1242086, @jfb wrote:

I think we should consider proposing this to the C committee. @aaron.ballman I can help Erik write the paper, would you be able to present it? Too tight for the upcoming meeting, but I'm guessing we have *plenty* of time for ++C17.

It's already been proposed to WG14 and is currently on the SD-3 list of features to consider for C2x. See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2008.pdf. I know Clive and am happy to point him towards this patch (when it lands) as demonstration of industry desire for the feature, in case he needs to provide updated papers.

Wonderful! Does this match he proposed C2x semantics? Once voted in we'll want to change this from just an extension to also be a -std=c2x thing, better have them match now.

[aaron.ballman]

In D52339#1242118, @jfb wrote:

In D52339#1242103, @aaron.ballman wrote:

In D52339#1242086, @jfb wrote:

I think we should consider proposing this to the C committee. @aaron.ballman I can help Erik write the paper, would you be able to present it? Too tight for the upcoming meeting, but I'm guessing we have *plenty* of time for ++C17.

It's already been proposed to WG14 and is currently on the SD-3 list of features to consider for C2x. See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2008.pdf. I know Clive and am happy to point him towards this patch (when it lands) as demonstration of industry desire for the feature, in case he needs to provide updated papers.

Wonderful! Does this match he proposed C2x semantics? Once voted in we'll want to change this from just an extension to also be a -std=c2x thing, better have them match now.

I have to validate that still, but from a quick look, I think we're in the ballpark if not outright compatible.

[erik.pilkington]

In D52339#1242126, @aaron.ballman wrote:

In D52339#1242118, @jfb wrote:

In D52339#1242103, @aaron.ballman wrote:

In D52339#1242086, @jfb wrote:

I think we should consider proposing this to the C committee. @aaron.ballman I can help Erik write the paper, would you be able to present it? Too tight for the upcoming meeting, but I'm guessing we have *plenty* of time for ++C17.

It's already been proposed to WG14 and is currently on the SD-3 list of features to consider for C2x. See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2008.pdf. I know Clive and am happy to point him towards this patch (when it lands) as demonstration of industry desire for the feature, in case he needs to provide updated papers.

Wonderful! Does this match he proposed C2x semantics? Once voted in we'll want to change this from just an extension to also be a -std=c2x thing, better have them match now.

I have to validate that still, but from a quick look, I think we're in the ballpark if not outright compatible.

From the last line in the paper, it seems that C++ compatibility is a goal of the paper (or at least a consideration). We should probably think about this if/when the final wording gets accepted though.

clang/include/clang/Basic/Features.def

90 ↗

(On Diff #166397)

I suppose it's a first class feature in ObjC2, but an extension in ObjC1. On second thought I should probably not change this, because ObjC1 doesn't have this as a feature, although it does have it as an extension.

233 ↗

(On Diff #166397)

I can't think of any reason why not, seems there are a handful of other EXTENSION(*, true) features. Do you have a preference?

[erik.pilkington]

clang/include/clang/Basic/Features.def

90 ↗

(On Diff #166397)

On third thought, it looks like we never even try to compile with ObjC1 = 1 and ObjC2 = 0, so I guess this is fine as-is.

[aaron.ballman]

In D52339#1242202, @erik.pilkington wrote:

From the last line in the paper, it seems that C++ compatibility is a goal of the paper (or at least a consideration). We should probably think about this if/when the final wording gets accepted though.

Agreed. WG14's charter explicitly prefers compatibility with C++ when possible.

The part that I wasn't quite sure on were the type constraints in the proposal. C++ allows any integral type and ignores cv qualifiers, but the proposal lists specific types and doesn't discuss qualifiers. By my reading, this is code allowed in C++ but prohibited in the proposed wording:

enum E : const int32_t {
  One
};

(Because the type is int32_t and is cv-qualified.) However, it's possible that's an oversight rather than an intentional design. I'll bring it up with Clive to see, but perhaps we can spot other divergences and can provide him with a list of concerns on the implementation side.

clang/include/clang/Basic/Features.def

233 ↗

(On Diff #166397)

I think we should probably ask folks from the projects to see if they're okay with the extension or not, but I'd guess this won't be contentious.

[rjmccall]

In D52339#1242241, @aaron.ballman wrote:

In D52339#1242202, @erik.pilkington wrote:

From the last line in the paper, it seems that C++ compatibility is a goal of the paper (or at least a consideration). We should probably think about this if/when the final wording gets accepted though.

Agreed. WG14's charter explicitly prefers compatibility with C++ when possible.

The part that I wasn't quite sure on were the type constraints in the proposal. C++ allows any integral type and ignores cv qualifiers, but the proposal lists specific types and doesn't discuss qualifiers. By my reading, this is code allowed in C++ but prohibited in the proposed wording:

enum E : const int32_t {
  One
};

(Because the type is int32_t and is cv-qualified.) However, it's possible that's an oversight rather than an intentional design. I'll bring it up with Clive to see, but perhaps we can spot other divergences and can provide him with a list of concerns on the implementation side.

Not accepting typedefs would be negligence on behalf of the committee, since generally people use this feature specifically to control the size of the enum, which you can't do portably without a typedef. Not accepting qualified types is a justifiable decision.

There's no reason to make this an ObjC2-only feature; we should probably eliminate that distinction throughout the compiler.

[erik.pilkington]

There's no reason to make this an ObjC2-only feature; we should probably eliminate that distinction throughout the compiler.

Sure, I was just writing a proposal to do that: http://lists.llvm.org/pipermail/cfe-dev/2018-September/059468.html

[erik.pilkington]

clang/include/clang/Basic/Features.def

233 ↗

(On Diff #166397)

Sure, @jlebar and @Anastasia: Any thoughts here?

[erik.pilkington]

Ping! This doesn't really affect CUDA or OpenCL more than any change to the compiler, so I don't think it makes sense to block this until we hear from them.

[aaron.ballman]

In D52339#1249432, @erik.pilkington wrote:

Ping! This doesn't really affect CUDA or OpenCL more than any change to the compiler, so I don't think it makes sense to block this until we hear from them.

It impacts them insomuch as they're standards and they may want to opt out of extensions for keeping code portable to other compilers.

However, short of that concern, this LGTM. I've spoken with the paper author for the WG14 paper and I think this approach fits their general proposal, so I don't think we'll have divergence here.

[rsmith]

In D52339#1249457, @aaron.ballman wrote:

In D52339#1249432, @erik.pilkington wrote:

Ping! This doesn't really affect CUDA or OpenCL more than any change to the compiler, so I don't think it makes sense to block this until we hear from them.

It impacts them insomuch as they're standards and they may want to opt out of extensions for keeping code portable to other compilers.

My view is that that's what -pedantic / -pedantic-errors are for. We want Clang to have a consistent user experience across languages, so if we're supporting fixed underlying types as an official Clang extension in languages that don't have it as a feature, that should apply everywhere it makes sense.

[aaron.ballman]

In D52339#1249531, @rsmith wrote:

In D52339#1249457, @aaron.ballman wrote:

In D52339#1249432, @erik.pilkington wrote:

Ping! This doesn't really affect CUDA or OpenCL more than any change to the compiler, so I don't think it makes sense to block this until we hear from them.

It impacts them insomuch as they're standards and they may want to opt out of extensions for keeping code portable to other compilers.

My view is that that's what -pedantic / -pedantic-errors are for. We want Clang to have a consistent user experience across languages, so if we're supporting fixed underlying types as an official Clang extension in languages that don't have it as a feature, that should apply everywhere it makes sense.

That sounds like a reasonable approach to me. I'll switch my LGTM to RLGTM (Really LGTM). :-)

[erik.pilkington]

In D52339#1249457, @aaron.ballman wrote:

However, short of that concern, this LGTM. I've spoken with the paper author for the WG14 paper and I think this approach fits their general proposal, so I don't think we'll have divergence here.

Okay, great! Thanks for verifying that.

[arphaman]

@erik.pilkington the change to make objc_fixed_enum true in non-ObjC mode turned out to be problematic for our adoption. Could you please fix it before LLVM8 is wrapped up so that we don't ship this extension as on by default for all language modes?

[arphaman]

We could introduce a new extension flag like c_fixed_enum that can be set for all language modes. objc_fixed_enum should apply to ObjC only.

[erik.pilkington]

In D52339#1377527, @arphaman wrote:

@erik.pilkington the change to make objc_fixed_enum true in non-ObjC mode turned out to be problematic for our adoption. Could you please fix it before LLVM8 is wrapped up so that we don't ship this extension as on by default for all language modes?

Yeah, now that I look again, we shouldn't be changing the FEATURE(...) flag behaviour, because fixed enums in C aren't a formal language feature. I removed this in r352672.

In D52339#1377536, @arphaman wrote:

We could introduce a new extension flag like c_fixed_enum that can be set for all language modes. objc_fixed_enum should apply to ObjC only.

Can't we just use cxx_fixed_enum?

include/clang/Basic/DiagnosticParseKinds.td

def warn_cxx98_compat_enumerator_list_comma : Warning<
   InGroup<CXX98CompatPedantic>, DefaultIgnore;
 def err_enumerator_list_missing_comma : Error<
   "missing ',' between enumerators">;
 def err_enumerator_unnamed_no_def : Error<
   "unnamed enumeration must be a definition">;
 def ext_cxx11_enum_fixed_underlying_type : Extension<
   "enumeration types with a fixed underlying type are a C++11 extension">,
   InGroup<CXX11>;
-def ext_c_enum_fixed_underlying_type : Extension<
+def ext_ms_c_enum_fixed_underlying_type : Extension<
   "enumeration types with a fixed underlying type are a Microsoft extension">,
   InGroup<MicrosoftFixedEnum>;
+def ext_clang_c_enum_fixed_underlying_type : Extension<
+  "enumeration types with a fixed underlying type are a Clang extension">,
+  InGroup<DiagGroup<"fixed-enum-extension">>;
 def warn_cxx98_compat_enum_fixed_underlying_type : Warning<
   "enumeration types with a fixed underlying type are incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
 def warn_cxx98_compat_alignof : Warning<
   "alignof expressions are incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup<GNUAlignofExpression>;

include/clang/Basic/Features.def

FEATURE(efficiency_sanitizer,
         LangOpts.Sanitize.hasOneOf(SanitizerKind::Efficiency))
 FEATURE(scudo, LangOpts.Sanitize.hasOneOf(SanitizerKind::Scudo))
 // Objective-C features
 FEATURE(objc_arr, LangOpts.ObjCAutoRefCount) // FIXME: REMOVE?
 FEATURE(objc_arc, LangOpts.ObjCAutoRefCount)
 FEATURE(objc_arc_fields, true)
 FEATURE(objc_arc_weak, LangOpts.ObjCWeak)
 FEATURE(objc_default_synthesize_properties, LangOpts.ObjC2)
-FEATURE(objc_fixed_enum, LangOpts.ObjC2)
+FEATURE(objc_fixed_enum, true)
 FEATURE(objc_instancetype, LangOpts.ObjC2)
 FEATURE(objc_kindof, LangOpts.ObjC2)
 FEATURE(objc_modules, LangOpts.ObjC2 &&LangOpts.Modules)
 FEATURE(objc_nonfragile_abi, LangOpts.ObjCRuntime.isNonFragile())
 FEATURE(objc_property_explicit_atomic, true)
 FEATURE(objc_protocol_qualifier_mangling, true)
 FEATURE(objc_weak_class, LangOpts.ObjCRuntime.hasWeakClassImport())
 FEATURE(ownership_holds, true)

 EXTENSION(cxx_inline_namespaces, LangOpts.CPlusPlus)
 EXTENSION(cxx_local_type_template_args, LangOpts.CPlusPlus)
 EXTENSION(cxx_nonstatic_member_init, LangOpts.CPlusPlus)
 EXTENSION(cxx_override_control, LangOpts.CPlusPlus)
 EXTENSION(cxx_range_for, LangOpts.CPlusPlus)
 EXTENSION(cxx_reference_qualified_functions, LangOpts.CPlusPlus)
 EXTENSION(cxx_rvalue_references, LangOpts.CPlusPlus)
 EXTENSION(cxx_variadic_templates, LangOpts.CPlusPlus)
+EXTENSION(cxx_fixed_enum, true)
 // C++14 features supported by other languages as extensions.
 EXTENSION(cxx_binary_literals, true)
 EXTENSION(cxx_init_captures, LangOpts.CPlusPlus11)
 EXTENSION(cxx_variable_templates, LangOpts.CPlusPlus)
 // Miscellaneous language extensions
 EXTENSION(overloadable_unmarked, true)
 
 #undef EXTENSION
 #undef FEATURE

lib/Parse/ParseDecl.cpp

void Parser::ParseEnumSpecifier(SourceLocation StartLoc, DeclSpec &DS,
   bool shouldDelayDiagsInTag =
     (TemplateInfo.Kind == ParsedTemplateInfo::ExplicitInstantiation ||
      TemplateInfo.Kind == ParsedTemplateInfo::ExplicitSpecialization);
   SuppressAccessChecks diagsFromTag(*this, shouldDelayDiagsInTag);
 
   // Enum definitions should not be parsed in a trailing-return-type.
   bool AllowDeclaration = DSC != DeclSpecContext::DSC_trailing;
 
-  bool AllowFixedUnderlyingType = AllowDeclaration &&
-    (getLangOpts().CPlusPlus11 || getLangOpts().MicrosoftExt ||
-     getLangOpts().ObjC2);
-
   CXXScopeSpec &SS = DS.getTypeSpecScope();
   if (getLangOpts().CPlusPlus) {
     // "enum foo : bar;" is not a potential typo for "enum foo::bar;"
     // if a fixed underlying type is allowed.
-    ColonProtectionRAIIObject X(*this, AllowFixedUnderlyingType);
+    ColonProtectionRAIIObject X(*this, AllowDeclaration);
 
     CXXScopeSpec Spec;
     if (ParseOptionalCXXScopeSpecifier(Spec, nullptr,
                                        /*EnteringContext=*/true))
       return;
 
     if (Spec.isSet() && Tok.isNot(tok::identifier)) {
       Diag(Tok, diag::err_expected) << tok::identifier;
       if (Tok.isNot(tok::l_brace)) {
         // Has no name and is not a definition.
         // Skip the rest of this declarator, up until the comma or semicolon.
         SkipUntil(tok::comma, StopAtSemi);
         return;
       }
     }
 
     SS = Spec;
   }
 
   // Must have either 'enum name' or 'enum {...}'.
   if (Tok.isNot(tok::identifier) && Tok.isNot(tok::l_brace) &&
-      !(AllowFixedUnderlyingType && Tok.is(tok::colon))) {
+      !(AllowDeclaration && Tok.is(tok::colon))) {
     Diag(Tok, diag::err_expected_either) << tok::identifier << tok::l_brace;
 
     // Skip the rest of this declarator, up until the comma or semicolon.
     SkipUntil(tok::comma, StopAtSemi);
     return;
   }
 
   // If an identifier is present, consume and remember it.
void Parser::ParseEnumSpecifier(SourceLocation StartLoc, DeclSpec &DS,
   // diagnostics in a second.
   if (shouldDelayDiagsInTag)
     diagsFromTag.done();
 
   TypeResult BaseType;
 
   // Parse the fixed underlying type.
   bool CanBeBitfield = getCurScope()->getFlags() & Scope::ClassScope;
-  if (AllowFixedUnderlyingType && Tok.is(tok::colon)) {
+  if (AllowDeclaration && Tok.is(tok::colon)) {
     bool PossibleBitfield = false;
     if (CanBeBitfield) {
       // If we're in class scope, this can either be an enum declaration with
       // an underlying type, or a declaration of a bitfield member. We try to
       // use a simple disambiguation scheme first to catch the common cases
       // (integer literal, sizeof); if it's still ambiguous, we then consider
       // anything that's a simple-type-specifier followed by '(' as an
       // expression. This suffices because function types are not valid
if (CanBeBitfield) {
       // Consume the ':'.
       ConsumeToken();
     }
 
     if (!PossibleBitfield) {
       SourceRange Range;
       BaseType = ParseTypeName(&Range);
 
-      if (getLangOpts().CPlusPlus11) {
+      if (!getLangOpts().ObjC2) {
+        if (getLangOpts().CPlusPlus11)
           Diag(StartLoc, diag::warn_cxx98_compat_enum_fixed_underlying_type);
-      } else if (!getLangOpts().ObjC2) {
-        if (getLangOpts().CPlusPlus)
-          Diag(StartLoc, diag::ext_cxx11_enum_fixed_underlying_type) << Range;
+        else if (getLangOpts().CPlusPlus)
+          Diag(StartLoc, diag::ext_cxx11_enum_fixed_underlying_type);
+        else if (getLangOpts().MicrosoftExt)
+          Diag(StartLoc, diag::ext_ms_c_enum_fixed_underlying_type);
         else
-          Diag(StartLoc, diag::ext_c_enum_fixed_underlying_type) << Range;
+          Diag(StartLoc, diag::ext_clang_c_enum_fixed_underlying_type);
       }
     }
   }
 
   // There are four options here.  If we have 'friend enum foo;' then this is a
   // friend declaration, and cannot have an accompanying definition. If we have
   // 'enum foo;', then this is a forward declaration.  If we have
   // 'enum foo {...' then this is a definition. Otherwise we have something

test/Sema/fixed-enum.c

+// RUN: %clang_cc1 -Weverything -xc++ -std=c++11 -DCXX11 -verify %s
+// RUN: %clang_cc1 -Weverything -xc++ -std=c++03 -DCXX03 -verify %s
+// RUN: %clang_cc1 -Weverything -xobjective-c -DOBJC -verify %s
+// RUN: %clang_cc1 -Weverything -std=c11 -xc -DC11 -verify %s
+// RUN: %clang_cc1 -Weverything -std=c11 -xc -fms-extensions -DMS -verify %s
+
+enum X : int {e};
+#if defined(CXX11)
+// expected-warning@-2{{enumeration types with a fixed underlying type are incompatible with C++98}}
+#elif defined(CXX03)
+// expected-warning@-4{{enumeration types with a fixed underlying type are a C++11 extension}}
+#elif defined(OBJC)
+// expected-no-diagnostics
+#elif defined(C11)
+// expected-warning@-8{{enumeration types with a fixed underlying type are a Clang extension}}
+#elif defined(MS)
+// expected-warning@-10{{enumeration types with a fixed underlying type are a Microsoft extension}}
+#endif

test/SemaObjC/enum-fixed-type.m

 // RUN: %clang_cc1 -fsyntax-only -pedantic -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -xc %s
 
 #if !__has_feature(objc_fixed_enum)
 #  error Enumerations with a fixed underlying type are not supported
 #endif
 
+#if !__has_extension(cxx_fixed_enum)
+#  error Enumerations with a fixed underlying type are not supported
+#endif
+
 typedef long Integer;
 
 typedef enum : Integer { Enumerator1, Enumerator2 } Enumeration;
 
 int array[sizeof(Enumeration) == sizeof(long)? 1 : -1];
 
 
 enum Color { Red, Green, Blue };