12

My understanding is that [Flag] enums are typically used for things that can be combined, where the individual values aren't mutually exclusive.

For example:

[Flags]
public enum SomeAttributes
{
    Foo = 1 << 0,
    Bar = 1 << 1,
    Baz = 1 << 2,
}

Where any SomeAttributes value can be a combination of Foo, Bar and Baz.

In a more complicated, real-life scenario, I use an enum to describe a DeclarationType:

[Flags]
public enum DeclarationType
{
    Project = 1 << 0,
    Module = 1 << 1,
    ProceduralModule = 1 << 2 | Module,
    ClassModule = 1 << 3 | Module,
    UserForm = 1 << 4 | ClassModule,
    Document = 1 << 5 | ClassModule,
    ModuleOption = 1 << 6,
    Member = 1 << 7,
    Procedure = 1 << 8 | Member,
    Function = 1 << 9 | Member,
    Property = 1 << 10 | Member,
    PropertyGet = 1 << 11 | Property | Function,
    PropertyLet = 1 << 12 | Property | Procedure,
    PropertySet = 1 << 13 | Property | Procedure,
    Parameter = 1 << 14,
    Variable = 1 << 15,
    Control = 1 << 16 | Variable,
    Constant = 1 << 17,
    Enumeration = 1 << 18,
    EnumerationMember = 1 << 19,
    Event = 1 << 20,
    UserDefinedType = 1 << 21,
    UserDefinedTypeMember = 1 << 22,
    LibraryFunction = 1 << 23 | Function,
    LibraryProcedure = 1 << 24 | Procedure,
    LineLabel = 1 << 25,
    UnresolvedMember = 1 << 26,
    BracketedExpression = 1 << 27,
    ComAlias = 1 << 28
}

Obviously a given Declaration can't be both a Variable and a LibraryProcedure - the two individual values can't be combined.. and they're not.

While these flags are extremely useful (it's very easy to verify whether a given DeclarationType is a Property or a Module), it feels "wrong" because the flags aren't really used for combining values, but rather for grouping them into "sub-types".

So I'm told that this is abusing enum flags - this answer essentially says that if I have a set of values applicable to apples and another set applicable to oranges, then I need a different enum type for apples and another one for oranges - the problem with that here is that I need all declarations to have a common interface, with DeclarationType being exposed in the base Declaration class: having a PropertyType enum wouldn't be useful at all.

Is this a sloppy/surprising/abusive design? If so, then how is that problem typically solved?

  • Consider how people are going to use objects of type DeclarationType. If I want to determine whether or not x is a subtype of y, I'm probably going to want to write that as x.IsSubtypeOf(y), not as x && y == y. – Tanner Swett Jun 7 '17 at 21:31
  • 1
    @TannerSwett That's quite exactly what x.HasFlag(DeclarationType.Member) does.... – Mathieu Guindon Jun 7 '17 at 22:22
  • Yes, that's true. But if your method is called HasFlag instead of IsSubtypeOf, then I need some other way of finding out that what it really means is "is subtype of". You could create an extension method, but as a user, what I would find least surprising is for DeclarationType to just be a struct that has IsSubtypeOf as a real method. – Tanner Swett Jun 7 '17 at 22:46
10

This is definitely abusing enums and flags! It might work for you, but anybody else reading the code is going to be mightily confused.

If I understand correctly, you have a hierarchical classification of declarations. This is far to much information to encode in a single enum. But there is an obvious alternative: Use classes and inheritance! So Member inherits from DeclarationType, Property inherits from Member and so on.

Enums are appropriate in some particular circumstances: If a value is always one of a limited number of options, or if it is any combination of a limited number of options (flags). Any information which is more complex or structured than this should be represented using objects.

Edit: In your "real-life scenario" it seems there are multiple places where behavior is selected depending on the value of the enum. This is really an antipattern, since you are using switch + enum as a "poor mans polymorphism". Just turn the enum value into distinct classes encapsulating the declaration-specific behavor, and your code will be much cleaner

  • Inheritance is a good idea but your answer implies a class per enum, which seems overkill. – Frank Hileman Jun 7 '17 at 21:54
  • @FrankHileman: The "leafs" in the hierarchy could be represented as enum values rather than classes, but I'm not sure it would be better. Depends if different behavior would be associated wit the different enum values, in which case distinct classes would be better. – JacquesB Jun 8 '17 at 9:38
  • 4
    The classification is not hierarchical, the combinations are pre-defined states. Using inheritance for this would be truly abuse, abuse of classes that is. With a class I expect at least some data or some behavior. A bunch of classes without either are... right, an enum. – Martin Maat Jun 8 '17 at 11:36
  • About that link to my repo: the per-type behavior has more to do with the ParserRuleContext generated class' type than with the enum. That code is trying to get the token position where to insert an As {Type} clause in the declaration; these ParserRuleContext-derived classes are generated by Antr4 per a grammar that defines the parser rules - I don't really control the parse tree nodes' [rather shallow] inheritance hierarchy, although I can leverage their partial-ness to make them implement interfaces, e.g. to make them expose some AsTypeClauseTokenPosition property.. lots of work. – Mathieu Guindon Jun 8 '17 at 16:49
6

I find this approach easy enough to read and understand. IMHO, it is not something to get confused about. That being said, I have some concerns about this approach:

  1. My main reservation is that there's no way to enforce this:

    Obviously a given Declaration can't be both a Variable and a LibraryProcedure - the two individual values can't be combined.. and they're not.

    While you do not declare above combination, this code

    var oops = DeclarationType.Variable | DeclarationType.LibraryProcedure;
    

    Is perfectly valid. And there is no way to catch those errors at compilation time.

  2. There is a limit to how much information you can encode in bit flags, which is what, 64 bits? For now you are getting dangerously close to size of int and if this enum keeps growing you might eventually just run out of bits...

Bottom line is, I think this is a valid approach, but I would be hesitant to use it for large/complex hierarchies of flags.

  • So thorough parser unit tests would address #1 then. FWIW the enum started as a standard no-flags enum some 3 years ago.. It's after growing tired of always checking for some specific values (say, in code inspections) that enum flags appeared. The list isn't really going to grow over time either, but yeah beating int capacity is a real concern nonetheless. – Mathieu Guindon Jun 8 '17 at 11:45
3

TL;DR Scroll to the very bottom.


From what I see, you are implementing a new language on top of C#. The enums seems to denote the type of an identifier (or, anything that has a name and that appears in the new language's source code), which seems to be applied to nodes that are to be added into a tree representation of the program.

In this particular situation, there are very few polymorphic behaviors between the different types of nodes. In other words, while it is necessary for the tree to be able to contain nodes of very different types (variants), the actual visitation of these nodes will basically resort to a giant if-then-else chain (or instanceof / is checks). These giant checks will likely happen in many different places across the project. This is the reason why enums may seem helpful, or, they're at least as helpful as instanceof / is checks.

Visitor pattern might still be useful. In other words, there are various coding styles that can be used instead of the giant chain of instanceof. However, if you want a discussion on the various benefits and drawbacks, you would have chosen to showcase a code example from the ugliest chain of instanceof in the project, instead of quibbling about enums.

This is not to say classes and inheritance hierarchy aren't useful. Quite the opposite. While there aren't any polymorphic behaviors that work across every declaration type (aside from the fact that every declaration must have a Name property), there are plenty of rich polymorphic behaviors shared by nearby siblings. For example, Function and Procedure probably share some behaviors (both being callable and accepting a list of typed input arguments), and PropertyGet will definitely inherit behaviors from Function (both having a ReturnType). You might use either enums or inheritance checks for the giant if-then-else chain, but the polymorphic behaviors, however fragmented, must still be implemented in classes.

There're many online advice against overuse of instanceof / is checks. Performance is not one of the reason. Rather, the reason is to prevent the programmer from organically discovering suitable polymorphic behaviors, as if instanceof / is is a crutch. But in your situation, you have no other choice, since these nodes have very little in common.

Now here are some concrete suggestions.


There are several ways of representing the non-leaf groupings.


Compare the following excerpt of your original code...

[Flags]
public enum DeclarationType
{
    Member = 1 << 7,
    Procedure = 1 << 8 | Member,
    Function = 1 << 9 | Member,
    Property = 1 << 10 | Member,
    PropertyGet = 1 << 11 | Property | Function,
    PropertyLet = 1 << 12 | Property | Procedure,
    PropertySet = 1 << 13 | Property | Procedure,
    LibraryFunction = 1 << 23 | Function,
    LibraryProcedure = 1 << 24 | Procedure,
}

to this modified version:

[Flags]
public enum DeclarationType
{
    Nothing = 0, // to facilitate bit testing

    // Let's assume Member is not a concrete thing, 
    // which means it doesn't need its own bit
    /* Member = 1 << 7, */

    // Procedure and Function are concrete things; meanwhile 
    // they can still have sub-types.
    Procedure = 1 << 8, 
    Function = 1 << 9, 
    Property = 1 << 10,

    PropertyGet = 1 << 11,
    PropertyLet = 1 << 12,
    PropertySet = 1 << 13,

    LibraryFunction = 1 << 23,
    LibraryProcedure = 1 << 24,

    // new
    Procedures = Procedure | PropertyLet | PropertySet | LibraryProcedure,
    Functions = Function | PropertyGet | LibraryFunction,
    Properties = PropertyGet | PropertyLet | PropertySet,
    Members = Procedures | Functions | Properties,
    LibraryMembers = LibraryFunction | LibraryProcedure 
}

This modified version avoids allocating bits toward non-concrete declaration types. Instead, non-concrete declaration types (abstract groupings of declaration types) simply have enum values which are the bitwise-or (union of the bits) across all of its children.

There is a caveat: if there is an abstract declaration type that has a single child, and if there's a need to distinguish between the abstract one (parent) from the concrete one (child), then the abstract one will still need its own bit.


One caveat that is specific to this question: a Property is initially an identifier (when you just see its name, without seeing how it is used in the code), but it may transmute into PropertyGet / PropertyLet / PropertySet as soon as you see how it is being used in the code. In other words, at different stages of parsing, you might either need to mark a Property identifier as being "this name refers to a property", and later change that to "this line of code is accessing this property in a certain way".

To resolve this caveat, you might need two sets of enums; one enum denotes what a name (identifier) is; another enum denotes what the code is trying to do (e.g. declaring the body of something; trying to use something in a certain way).


Consider whether the auxiliary information about each enum value can be read off from an array instead.

This suggestion is mutually exclusive with other suggestions because it requires converting powers-of-two values back to small non-negative integer values.

public enum DeclarationType
{
    Procedure = 8,
    Function = 9,
    Property = 10,
    PropertyGet = 11,
    PropertyLet = 12,
    PropertySet = 13,
    LibraryFunction = 23,
    LibraryProcedure = 24,
}

static readonly bool[] DeclarationTypeIsMember = new bool[32]
{
    ?, ?, ?, ?, ?, ?, ?, ?,                   // bit[0] ... bit[7]
    true, true, true, true, true, true, ?, ?, // bit[8] ... bit[15]
    ?, ?, ?, ?, ?, ?, ?, true,                // bit[16] ... bit[23]
    true, ...                                 // bit[24] ... 
}

static bool IsMember(DeclarationType dt)
{
    int intValue = (int)dt;
    return (intValue < 0 || intValue >= 32) ? false : DeclarationTypeIsMember[intValue];
    // you can also throw an exception if the enum is outside range.
}

// likewise for IsFunction(dt), IsProcedure(dt), IsProperty(dt), ...

Maintainability is going to be problematic.


Check whether a one-to-one mapping between C# types (classes in an inheritance hierarchy) and your enum values.

(Alternatively, you can tweak your enum values to ensure a one-to-one mapping with types.)

In C#, a lot of libraries abuse the nifty Type object.GetType() method, for good or bad.

Anywhere you're storing the enum as a value, you might ask yourself whether you can store the Type as a value instead.

To use this trick, you can initialize two read-only hash tables, namely:

// For disambiguation, I'll assume that the actual 
// (behavior-implementing) classes are under the 
// "Lang" namespace.

static readonly Dictionary<Type, DeclarationType> TypeToDeclEnum = ... 
{
    { typeof(Lang.Procedure), DeclarationType.Procedure },
    { typeof(Lang.Function), DeclarationType.Function },
    { typeof(Lang.Property), DeclarationType.Property },
    ...
};

static readonly Dictionary<DeclarationType, Type> DeclEnumToType = ...
{
    // same as the first dictionary; 
    // just swap the key and the value
    ...
};

The final vindication for those suggesting classes and inheritance hierarchy...

Once you can see that the enums are an approximation to the inheritance hierarchy, the following advice holds:

  • Design (or improve) your inheritance hierarchy first,
  • Then go back and design your enums to approximate that inheritance hierarchy.
  • The project is a VBIDE add-in actually - I'm parsing and analyzing VBA code =) – Mathieu Guindon Jun 8 '17 at 12:30
1

I find your use of flags really smart, creative, elegant and potentially most efficient. I have no trouble at all reading it either.

Flags are a means of signaling state, of qualifying. If I want to know if something is fruit, I find

thingy & Organic.Fruit != 0

more readable than

thingy & (Organic.Apple | Organic.Orange | Organic.Pear) != 0

The very point of Flag enums is to allow you to combine multiple states. You just made this more useful and readable. You convey the concept of fruit in your code, I do not have to figure out myself that apple and orange and pear means fruit.

Give this guy some brownie points!

  • 4
    thingy is Fruit is more readable than either though. – JacquesB Jun 8 '17 at 10:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.