[PATCH] Rename flag_enum to enum_role
Aaron Ballman
aaron.ballman at gmail.com
Mon Jan 5 11:00:25 PST 2015
On Mon, Jan 5, 2015 at 1:09 PM, Sean Hunt <scshunt at csclub.uwaterloo.ca> wrote:
> Hi aaron.ballman,
>
> 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.
>
> http://reviews.llvm.org/D6847
>
> Files:
> include/clang/Basic/Attr.td
> include/clang/Basic/AttrDocs.td
> include/clang/Sema/Sema.h
> lib/Sema/SemaDecl.cpp
> lib/Sema/SemaDeclAttr.cpp
> lib/Sema/SemaStmt.cpp
> test/Sema/attr-enum-role.c
> test/Sema/attr-flag-enum.c
> test/SemaCXX/attr-enum-role.cpp
>
> EMAIL PREFERENCES
> http://reviews.llvm.org/settings/panel/emailpreferences/
Thank you for this! I have some comments and questions below.
> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td
> +++ include/clang/Basic/Attr.td
> @@ -675,6 +675,28 @@
> let Documentation = [EnableIfDocs];
> }
>
> +def EnumRole : InheritableAttr {
> + let Spellings = [GNU<"enum_role">];
Has any thought been put into a C++11 spelling yet? (I know this is
still a C-only attribute, but since you're continuing the work, I
figured I would prod a bit.)
> + let Subjects = SubjectList<[Enum]>;
> + let Documentation = [EnumRoleDocs];
> + let LangOpts = [COnly];
> + let Args = [EnumArgument<"Role", "EnumRole",
> + ["choice", "options"],
> + ["ER_Choice", "ER_Options"]>];
I'm not keen on the fake namespacing of the enumerators. There's only
one other attribute with an EnumArgument which does so -- the vast
majority leave off the Foo_ bit.
The user-facing names seem a bit obtuse, as "choice" and "options" are
kind of interchangeable terms. What about ExclusiveChoices and
BitwiseFlags, or something a bit more wordy?
> + 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>;
> @@ -709,25 +731,6 @@
> 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>;
> Index: include/clang/Basic/AttrDocs.td
> ===================================================================
> --- include/clang/Basic/AttrDocs.td
> +++ include/clang/Basic/AttrDocs.td
> @@ -1196,13 +1196,36 @@
> }];
> }
>
> -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
I don't see this as being the default in Attr.td -- the arguments are
required from everything I can tell.
> +used as a set of distinct values, and a value outside the defined enumerators is
> +invalid.
What about repeated values within the defined enumerators? The
behavior described doesn't seem quite like a choice so much as
containment within a set.
> +
> +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,
> + };
> }];
> }
>
> Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h
> +++ include/clang/Sema/Sema.h
> @@ -7966,11 +7966,11 @@
> 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,
> - bool AllowMask) const;
> + /// 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.
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -13505,26 +13505,27 @@
> }
>
> bool
> -Sema::IsValueInFlagEnum(const EnumDecl *ED, const llvm::APInt &Val,
> - bool AllowMask) const {
> - FlagEnumAttr *FEAttr = ED->getAttr<FlagEnumAttr>();
> - assert(FEAttr && "looking for value in non-flag enum");
> +Sema::IsValueInOptionsEnum(const EnumDecl *ED, const llvm::APInt &Val,
> + bool AllowMask) const {
> + 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) {
> @@ -13540,7 +13541,7 @@
> // 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;
> }
>
> @@ -13698,9 +13699,9 @@
> }
> }
>
> - 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
> @@ -13767,28 +13768,27 @@
> 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);
>
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -1024,6 +1024,36 @@
> 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;
> + }
This should be tablegennable some day, but that's not your problem. :-)
> +
> + 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;
> + }
This isn't of particular importance to most other attributes (barring
C++11 attributes within the same attribute-list, but not
attribute-specifier-seq). Why is it important here? I can certainly
understanding conflicting roles, but identical roles don't seem like
they should be diagnosed on.
> +
> + 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.
> @@ -4403,6 +4433,9 @@
> 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;
> @@ -4412,9 +4445,6 @@
> 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;
> Index: lib/Sema/SemaStmt.cpp
> ===================================================================
> --- lib/Sema/SemaStmt.cpp
> +++ lib/Sema/SemaStmt.cpp
> @@ -697,8 +697,6 @@
> 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())) {
> @@ -710,8 +708,9 @@
> }
> }
>
> - 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++;
> @@ -1198,8 +1197,9 @@
> 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 {
> Index: test/Sema/attr-enum-role.c
> ===================================================================
> --- /dev/null
> +++ test/Sema/attr-enum-role.c
> @@ -0,0 +1,90 @@
> +// 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}}
> +}
> Index: test/Sema/attr-flag-enum.c
> ===================================================================
> --- test/Sema/attr-flag-enum.c
> +++ /dev/null
> @@ -1,73 +0,0 @@
> -// 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}}
> -}
> Index: test/SemaCXX/attr-enum-role.cpp
> ===================================================================
> --- /dev/null
> +++ test/SemaCXX/attr-enum-role.cpp
> @@ -0,0 +1,3 @@
> +// RUN: %clang_cc1 -verify -fsyntax-only -std=c++14 %s
> +
> +enum __attribute__((enum_role(options))) wrong_lang { yerp }; // expected-warning {{attribute ignored}}
>
I have some test cases with questions:
enum __attribute__((enum_role(options))) E; // Perhaps we want to
disallow on forward declares?
enum __attribute__((enum_role(choice))) E { // Should probably warn
here if we do allow on forward declares?
ga = 0x1,
gb = 0x4,
gd = 0x7,
};
enum __attribute__((enum_role(choice))) E2 {
a = 1,
b = 1, // Should this warn? I can see arguments either way...
};
~Aaron
More information about the cfe-commits
mailing list