[PATCH] Rename flag_enum to enum_role

[Sean Hunt]

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.

>
> > +  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 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.

> 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.

Sean