[PATCH] Rename flag_enum to enum_role

[Aaron Ballman]

On Thu, Jan 8, 2015 at 4:44 PM, Sean Hunt <scshunt at csclub.uwaterloo.ca> wrote:
> On Mon, Jan 5, 2015 at 2:00 PM, Aaron Ballman <aaron.ballman at gmail.com> wrote:
>>
>>
>> > 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.)
>
>
> A little bit, but not really. C++ does a couple of things differently
> with enums so the semantics need to be thought about carefully.
>
>> > +  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?
>
>
> Fake namespacing is easy to kill. I guess I just saw that one argument
> and went after that style. As for the user-facing names, they're what
> dgregor suggested and I couldn't think of anything better. But any
> alternate suggestions from anyone would be appreciated! I'm not sure
> I'm a huge fan of the long names, though.

I'm a fan of self-documenting code, and since this is an extension, I
feel that "choice" and "options" are not particularly ideal. One thing
which bugged me when I was playing around with this is that I have to
remember which is plural and which isn't. Beyond that, I find them
obtuse since they're basically synonyms of sorts.

>
>>
>> > +  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.
>
> Good catch. Default meaning it's the behaviour if no attribute is specified.
>
>> > +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.
>
> Another good catch.
>
>> > +  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.
>
> Hmm, I'm a fan of diagnosing redundant attributes since I believe that
> the C++ standard forbids some, but I can look into this more. It's
> just a bikeshed though, so I'm not majorly attached one way or
> another.

I bring it up because we don't diagnose GNU attributes that are
duplicated unless there's some sort of conflict. The C++ attributes do
get diagnosed when duplicated, but only under certain circumstances,
and only for standards-defined attributes.

>
>
>> 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,
>> };
>
> C doesn't allow on forward declares.

That's news to me. The following compiles just fine with clang and gcc:

enum __attribute__((deprecated)) e;

enum e { one, two };

int main(void) {
  enum e e1 = one;
  return 0;
}

>
>> enum __attribute__((enum_role(choice))) E2 {
>>   a = 1,
>>   b = 1, // Should this warn? I can see arguments either way...
>> };
>>
>> ~Aaron
>
> No, because currently, "choice" is intended to mean "what we do
> currently". If we wanted an enum role for distinct options, we could
> add that, but given that it's not uncommon to do something like:
>
> enum foo {
>   a,
>   b,
>   c,
>   count = c,
> };
>
> that may prove to be more annoying than anything.

Fair enough. Thanks!

~Aaron