Rename flag_enum to enum_role

[scshunt]

Summary

Per discussions with dgregor, rename the flag_enum attribute to enum_role, and make it more extensible by accepting an argument. I've also made the test coverage a bit more comprehensive.

[silvas]

A couple drive-by comments; not trying to formally become a reviewer on the patch.

lib/Sema/SemaDecl.cpp

13786

Should we change the name of diag::warn_flag_enum_constant_out_of_range if we are no longer using the flag_enum nomenclature?

test/Sema/attr-flag-enum.c

3

Just doublechecking: we are no longer accepting flag_enum, right? Is there a potential compatibility issue?

include/clang/Basic/Attr.td

   let Documentation = [EnableIfDocs];
 }
 
+def EnumRole : InheritableAttr {
+  let Spellings = [GNU<"enum_role">];
+  let Subjects = SubjectList<[Enum]>;
+  let Documentation = [EnumRoleDocs];
+  let LangOpts = [COnly];
+  let Args = [EnumArgument<"Role", "EnumRole",
+               ["choice", "options"],
+               ["ER_Choice", "ER_Options"]>];
+  let AdditionalMembers = [{
+private:
+    llvm::APInt OptionBits;
+public:
+    llvm::APInt &getOptionBits() {
+      return OptionBits;
+    }
+
+    const llvm::APInt &getOptionBits() const {
+      return OptionBits;
+    }
+}];
+}
+
 def ExtVectorType : Attr {
   let Spellings = [GNU<"ext_vector_type">];
   let Subjects = SubjectList<[TypedefName], ErrorDiag>;

   let Documentation = [Undocumented];
 }
 
-def FlagEnum : InheritableAttr {
-  let Spellings = [GNU<"flag_enum">];
-  let Subjects = SubjectList<[Enum]>;
-  let Documentation = [FlagEnumDocs];
-  let LangOpts = [COnly];
-  let AdditionalMembers = [{
-private:
-    llvm::APInt FlagBits;
-public:
-    llvm::APInt &getFlagBits() {
-      return FlagBits;
-    }
-
-    const llvm::APInt &getFlagBits() const {
-      return FlagBits;
-    }
-}];
-}
-
 def Flatten : InheritableAttr {
   let Spellings = [GCC<"flatten">];
   let Subjects = SubjectList<[Function], ErrorDiag>;

include/clang/Basic/AttrDocs.td

   }];
 }
 
-def FlagEnumDocs : Documentation {
+def EnumRoleDocs : Documentation {
   let Category = DocCatType;
   let Content = [{
-This attribute can be added to an enumerator to signal to the compiler that it
-is intended to be used as a flag type. This will cause the compiler to assume
-that the range of the type includes all of the values that you can get by
-manipulating bits of the enumerator when issuing warnings.
+The enum_role attribute can be added to an enum declaration to provide a hint to
+the compiler about its meaning. Currently, two roles are recognized.
+
+The 'choice' role, which is the default, means that the enum is intended to be
+used as a set of distinct values, and a value outside the defined enumerators is
+invalid.
+
+The 'options' role means that the enum is intended to be used as a set of
+bitwise options, and a valid value may have any or all of the enumerator bits
+set.
+
+   .. code-block:: c
+
+     enum __attribute__((enum_role(choice))) choice {
+       choice_1 = 0,
+       choice_2 = 1,
+       choice_3 = 2,
+       choice_4 = 3,
+     };
+
+     enum __attribute__((enum_role(options))) options {
+       option_1 = 0x1,
+       option_2 = 0x2,
+       option_3 = 0x4,
+       option_4 = 0x8,
+       option_5 = 0x10,
+     };
   }];
 }
 

include/clang/Sema/Sema.h

                                 Expr *SrcExpr, AssignmentAction Action,
                                 bool *Complained = nullptr);
 
-  /// IsValueInFlagEnum - Determine if a value is allowed as part of a flag
-  /// enum. If AllowMask is true, then we also allow the complement of a valid
-  /// value, to be used as a mask.
-  bool IsValueInFlagEnum(const EnumDecl *ED, const llvm::APInt &Val,
+  /// IsValueInOptionsEnum - Determine if a value is allowed as part of an
+  /// options enum. If AllowMask is true, then we also allow the complement of a
+  /// valid value, to be used as a mask.
+  bool IsValueInOptionsEnum(const EnumDecl *ED, const llvm::APInt &Val,
                             bool AllowMask) const;
 
   /// DiagnoseAssignmentEnum - Warn if assignment to enum is a constant
   /// integer not in the range of enum values.

lib/Sema/SemaDecl.cpp

 }
 
 bool
-Sema::IsValueInFlagEnum(const EnumDecl *ED, const llvm::APInt &Val,
+Sema::IsValueInOptionsEnum(const EnumDecl *ED, const llvm::APInt &Val,
                            bool AllowMask) const {
-  FlagEnumAttr *FEAttr = ED->getAttr<FlagEnumAttr>();
-  assert(FEAttr && "looking for value in non-flag enum");
+  EnumRoleAttr *ERAttr = ED->getAttr<EnumRoleAttr>();
+  assert(ERAttr && ERAttr->getRole() == EnumRoleAttr::ER_Options &&
+         "looking for value in non-flag enum");
 
-  llvm::APInt FlagMask = ~FEAttr->getFlagBits();
-  unsigned Width = FlagMask.getBitWidth();
+  llvm::APInt OptionsMask = ~ERAttr->getOptionBits();
+  unsigned Width = OptionsMask.getBitWidth();
 
   // We will try a zero-extended value for the regular check first.
   llvm::APInt ExtVal = Val.zextOrSelf(Width);
 
-  // A value is in a flag enum if either its bits are a subset of the enum's
-  // flag bits (the first condition) or we are allowing masks and the same is
+  // A value is in an options enum if either its bits are a subset of the enum's
+  // foptionsbits (the first condition) or we are allowing masks and the same is
   // true of its complement (the second condition). When masks are allowed, we
   // allow the common idiom of ~(enum1 | enum2) to be a valid enum value.
   //
   // While it's true that any value could be used as a mask, the assumption is
   // that a mask will have all of the insignificant bits set. Anything else is
   // likely a logic error.
-  if (!(FlagMask & ExtVal))
+  if (!(OptionsMask & ExtVal))
     return true;
 
   if (AllowMask) {

     // though, it can be fixed by making it a signed type (e.g. ~0x1), so it may
     // be fine just to accept this as a warning.
     ExtVal |= llvm::APInt::getHighBitsSet(Width, Width - Val.getBitWidth());
-    if (!(FlagMask & ~ExtVal))
+    if (!(OptionsMask & ~ExtVal))
       return true;
   }
 

     }
   }
 
-  FlagEnumAttr *FEAttr = Enum->getAttr<FlagEnumAttr>();
-  if (FEAttr)
-    FEAttr->getFlagBits() = llvm::APInt(BestWidth, 0);
+  EnumRoleAttr *ERAttr = Enum->getAttr<EnumRoleAttr>();
+  if (ERAttr)
+    ERAttr->getOptionBits() = llvm::APInt(BestWidth, 0);
 
   // Loop over all of the enumerator constants, changing their types to match
   // the type of the enum if needed. If we have a flag type, we also prepare the

 flagbits:
     // Check to see if we have a constant with exactly one bit set. Note that x
     // & (x - 1) will be nonzero if and only if x has more than one bit set.
-    if (FEAttr) {
+    if (ERAttr && ERAttr->getRole() == EnumRoleAttr::ER_Options) {
       llvm::APInt ExtVal = InitVal.zextOrSelf(BestWidth);
       if (ExtVal != 0 && !(ExtVal & (ExtVal - 1))) {
-        FEAttr->getFlagBits() |= ExtVal;
+        ERAttr->getOptionBits() |= ExtVal;
       }
     }
   }
 
-  if (FEAttr) {
+  if (ERAttr && ERAttr->getRole() == EnumRoleAttr::ER_Options) {
     for (Decl *D : Elements) {
       EnumConstantDecl *ECD = cast_or_null<EnumConstantDecl>(D);
       if (!ECD) continue;  // Already issued a diagnostic.
 
       llvm::APSInt InitVal = ECD->getInitVal();
-      if (InitVal != 0 && !IsValueInFlagEnum(Enum, InitVal, true))
+      if (InitVal != 0 && !IsValueInOptionsEnum(Enum, InitVal, true))
         Diag(ECD->getLocation(), diag::warn_flag_enum_constant_out_of_range)
           << ECD << Enum;
     }
   }
 
 
-
   Enum->completeDefinition(BestType, BestPromotionType,
                            NumPositiveBits, NumNegativeBits);
 

lib/Sema/SemaDeclAttr.cpp

                                 Attr.getAttributeSpellingListIndex()));
 }
 
+static void handleEnumRoleAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+  if (!Attr.isArgIdent(0)) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_type)
+      << Attr.getName() << 1 << AANT_ArgumentIdentifier;
+    return;
+  }
+
+  IdentifierLoc *IL = Attr.getArgAsIdent(0);
+  EnumRoleAttr::EnumRole ER;
+  if (!EnumRoleAttr::ConvertStrToEnumRole(IL->Ident->getName(), ER)) {
+    S.Diag(IL->Loc, diag::warn_attribute_type_not_supported) << Attr.getName()
+      << IL->Ident;
+    return;
+  }
+
+  if (EnumRoleAttr *ERAttr = D->getAttr<EnumRoleAttr>()) {
+    if (ERAttr->getRole() == ER)
+      S.Diag(Attr.getLoc(), diag::warn_duplicate_attribute_exact)
+        << Attr.getName();
+    else
+      S.Diag(Attr.getLoc(), diag::warn_duplicate_attribute) << Attr.getName();
+
+    return;
+  }
+
+  D->addAttr(::new (S.Context)
+             EnumRoleAttr(Attr.getLoc(), S.Context, ER,
+                          Attr.getAttributeSpellingListIndex()));
+}
+
 static void handleExtVectorTypeAttr(Sema &S, Scope *scope, Decl *D,
                                     const AttributeList &Attr) {
   // Remember this typedef decl, we will need it later for diagnostics.

   case AttributeList::AT_EnableIf:
     handleEnableIfAttr(S, D, Attr);
     break;
+  case AttributeList::AT_EnumRole:
+    handleEnumRoleAttr(S, D, Attr);
+    break;
   case AttributeList::AT_ExtVectorType:
     handleExtVectorTypeAttr(S, scope, D, Attr);
     break;

   case AttributeList::AT_OptimizeNone:
     handleOptimizeNoneAttr(S, D, Attr);
     break;
-  case AttributeList::AT_FlagEnum:
-    handleSimpleAttribute<FlagEnumAttr>(S, D, Attr);
-    break;
   case AttributeList::AT_Flatten:
     handleSimpleAttribute<FlattenAttr>(S, D, Attr);
     break;

lib/Sema/SemaStmt.cpp

                                               EnumValsTy::iterator &EI,
                                               EnumValsTy::iterator &EIEnd,
                                               const llvm::APSInt &Val) {
-  bool FlagType = ED->hasAttr<FlagEnumAttr>();
-
   if (const DeclRefExpr *DRE =
           dyn_cast<DeclRefExpr>(CaseExpr->IgnoreParenImpCasts())) {
     if (const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl())) {

     }
   }
 
-  if (FlagType) {
-    return !S.IsValueInFlagEnum(ED, Val, false);
+  EnumRoleAttr *ERAttr = ED->getAttr<EnumRoleAttr>();
+  if (ERAttr && ERAttr->getRole() == EnumRoleAttr::ER_Options) {
+    return !S.IsValueInOptionsEnum(ED, Val, false);
   } else {
     while (EI != EIEnd && EI->first < Val)
       EI++;

         AdjustAPSInt(RhsVal, DstWidth, DstIsSigned);
         const EnumDecl *ED = ET->getDecl();
 
-        if (ED->hasAttr<FlagEnumAttr>()) {
-          if (!IsValueInFlagEnum(ED, RhsVal, true))
+        EnumRoleAttr *ERAttr = ED->getAttr<EnumRoleAttr>();
+        if (ERAttr && ERAttr->getRole() == EnumRoleAttr::ER_Options) {
+          if (!IsValueInOptionsEnum(ED, RhsVal, true))
             Diag(SrcExpr->getExprLoc(), diag::warn_not_in_enum_assignment)
               << DstType.getUnqualifiedType();
         } else {

test/Sema/attr-enum-role.c

+// RUN: %clang_cc1 -verify -fsyntax-only -std=c11 -Wassign-enum %s
+
+__attribute__((enum_role(options))) int i; // expected-warning {{only applies to enums}}
+typedef struct __attribute__((enum_role(choice))) s { } t; // expected-warning {{only applies to enums}}
+
+enum __attribute__((enum_role(options))) flag {
+  ea = 0x1,
+  eb = 0x2,
+  ec = 0x8,
+};
+
+enum __attribute__((enum_role(options))) flag2 {
+  ga = 0x1,
+  gb = 0x4,
+
+  gc = 0x5, // no-warning
+  gd = 0x7, // expected-warning {{enumeration value 'gd' is out of range}}
+  ge = ~0x2, // expected-warning {{enumeration value 'ge' is out of range}}
+  gf = ~0x4, // no-warning
+  gg = ~0x1, // no-warning
+  gh = ~0x5, // no-warning
+  gi = ~0x11, // expected-warning {{enumeration value 'gi' is out of range}}
+};
+
+enum __attribute__((enum_role(options))) flag3 {
+  fa = 0x1,
+  fb = ~0x1u, // no-warning
+};
+
+// What happens here is that ~0x2 is negative, and so the enum must be signed.
+// But ~0x1u is unsigned and has the high bit set, so the enum must be 64-bit.
+// The result is that ~0x1u does not have high bits set, and so it is considered
+// to be an invalid value. See Sema::IsValueInOptionsEnum in SemaDecl.cpp for
+// more discussion.
+enum __attribute__((enum_role(options))) flag4 {
+  ha = 0x1,
+  hb = 0x2,
+
+  hc = ~0x1u, // expected-warning {{enumeration value 'hc' is out of range}}
+  hd = ~0x2, // no-warning
+};
+
+enum __attribute__((enum_role(choice))) choice {
+  za = 0x1,
+  zb = 0x2,
+};
+
+enum __attribute__((enum_role)) invalid1 { inv1 }; // expected-error {{takes one argument}}
+enum __attribute__((enum_role(foo))) invalid2 { inv2 }; // expected-warning {{argument not supported}}
+enum __attribute__((enum_role(1))) invalid3 { inv3 }; // expected-error {{requires parameter 1 to be an identifier}}
+
+enum __attribute__((enum_role(options), enum_role(options))) dupgood { optionforsure }; // expected-warning {{already applied}}
+enum __attribute__((enum_role(options), enum_role(choice))) dupbad { confusion }; // expected-warning {{already applied with different parameters}}
+
+void f(void) {
+  enum flag e = 0; // no-warning
+  e = 0x1; // no-warning
+  e = 0x3; // no-warning
+  e = 0xa; // no-warning
+  e = 0x4; // expected-warning {{integer constant not in range of enumerated type}}
+  e = 0xf; // expected-warning {{integer constant not in range of enumerated type}}
+  e = ~0; // no-warning
+  e = ~0x1; // no-warning
+  e = ~0x2; // no-warning
+  e = ~0x3; // no-warning
+  e = ~0x4; // expected-warning {{integer constant not in range of enumerated type}}
+
+  switch (e) {
+    case 0: break; // no-warning
+    case 0x1: break; // no-warning
+    case 0x3: break; // no-warning
+    case 0xa: break; // no-warning
+    case 0x4: break; // expected-warning {{case value not in enumerated type}}
+    case 0xf: break; // expected-warning {{case value not in enumerated type}}
+    case ~0: break; // expected-warning {{case value not in enumerated type}}
+    case ~0x1: break; // expected-warning {{case value not in enumerated type}}
+    case ~0x2: break; // expected-warning {{case value not in enumerated type}}
+    case ~0x3: break; // expected-warning {{case value not in enumerated type}}
+    case ~0x4: break; // expected-warning {{case value not in enumerated type}}
+    default: break;
+  }
+
+  enum flag2 f = ~0x1; // no-warning
+  f = ~0x1u; // no-warning
+
+  enum flag4 h = ~0x1; // no-warning
+  h = ~0x1u; // expected-warning {{integer constant not in range of enumerated type}}
+
+  enum choice z = 0x3; // expected-warning {{integer constant not in range of enumerated type}}
+}

test/Sema/attr-flag-enum.c

-// RUN: %clang_cc1 -verify -fsyntax-only -std=c11 -Wassign-enum %s
-
-enum __attribute__((flag_enum)) flag {
-  ea = 0x1,
-  eb = 0x2,
-  ec = 0x8,
-};
-
-enum __attribute__((flag_enum)) flag2 {
-  ga = 0x1,
-  gb = 0x4,
-
-  gc = 0x5, // no-warning
-  gd = 0x7, // expected-warning {{enumeration value 'gd' is out of range}}
-  ge = ~0x2, // expected-warning {{enumeration value 'ge' is out of range}}
-  gf = ~0x4, // no-warning
-  gg = ~0x1, // no-warning
-  gh = ~0x5, // no-warning
-  gi = ~0x11, // expected-warning {{enumeration value 'gi' is out of range}}
-};
-
-enum __attribute__((flag_enum)) flag3 {
-  fa = 0x1,
-  fb = ~0x1u, // no-warning
-};
-
-// What happens here is that ~0x2 is negative, and so the enum must be signed.
-// But ~0x1u is unsigned and has the high bit set, so the enum must be 64-bit.
-// The result is that ~0x1u does not have high bits set, and so it is considered
-// to be an invalid value. See Sema::IsValueInFlagEnum in SemaDecl.cpp for more
-// discussion.
-enum __attribute__((flag_enum)) flag4 {
-  ha = 0x1,
-  hb = 0x2,
-
-  hc = ~0x1u, // expected-warning {{enumeration value 'hc' is out of range}}
-  hd = ~0x2, // no-warning
-};
-
-void f(void) {
-  enum flag e = 0; // no-warning
-  e = 0x1; // no-warning
-  e = 0x3; // no-warning
-  e = 0xa; // no-warning
-  e = 0x4; // expected-warning {{integer constant not in range of enumerated type}}
-  e = 0xf; // expected-warning {{integer constant not in range of enumerated type}}
-  e = ~0; // no-warning
-  e = ~0x1; // no-warning
-  e = ~0x2; // no-warning
-  e = ~0x3; // no-warning
-  e = ~0x4; // expected-warning {{integer constant not in range of enumerated type}}
-
-  switch (e) {
-    case 0: break; // no-warning
-    case 0x1: break; // no-warning
-    case 0x3: break; // no-warning
-    case 0xa: break; // no-warning
-    case 0x4: break; // expected-warning {{case value not in enumerated type}}
-    case 0xf: break; // expected-warning {{case value not in enumerated type}}
-    case ~0: break; // expected-warning {{case value not in enumerated type}}
-    case ~0x1: break; // expected-warning {{case value not in enumerated type}}
-    case ~0x2: break; // expected-warning {{case value not in enumerated type}}
-    case ~0x3: break; // expected-warning {{case value not in enumerated type}}
-    case ~0x4: break; // expected-warning {{case value not in enumerated type}}
-    default: break;
-  }
-
-  enum flag2 f = ~0x1; // no-warning
-  f = ~0x1u; // no-warning
-
-  enum flag4 h = ~0x1; // no-warning
-  h = ~0x1u; // expected-warning {{integer constant not in range of enumerated type}}
-}

test/SemaCXX/attr-enum-role.cpp

+// RUN: %clang_cc1 -verify -fsyntax-only -std=c++14 %s
+
+enum __attribute__((enum_role(options))) wrong_lang { yerp }; // expected-warning {{attribute ignored}}