EnumToFlags

[ag0aep6g via Digitalmars-d-learn]

On 06/30/2016 04:39 AM, JS wrote:
> struct EnumToFlags(alias E)
> {
>      import std.traits, std.conv, std.string, std.algorithm, std.array;
>
>      static if (E.max < 8)
>          alias vtype = ubyte;

Convention says: Capitalize user-defined type names. So it should be 
"Vtype" or maybe "VType" instead of "vtype".

>      else static if (E.max < 16)
>          alias vtype = ushort;
>      else static if (E.max < 32)
>          alias vtype = uint;
>      else static if (E.max < 64)
>          alias vtype = ulong;
>      else static if (E.max < 128)
>          alias vtype = ucent;

There is no ucent type. The keyword is reserved, but there is no actual 
type.

>      else static assert("Cannot store more than 128 flags in an enum.");
>
>      vtype value;
>
>
>      template opDispatch(string Name)
>      {
>          enum opDispatch = 1 << __traits(getMember, E, Name);

`1` needs to be `1L` here, or maybe `1UL`. Can't shift more than 31 bits 
otherwise, because `1` is an int. Same throughout.

Note that you're shifting by the enum member's value here.

>      }
>
>
>      auto opAssign(vtype e)
>      {
>          value = e;
>      }

In my opinion, a void return type would be clearer here.

>
>      bool opEquals(typeof(this) e)
>      {
>          if (e is this) return true;
>          return (value == e.value);
>      }

opEquals should be const. The parentheses around the return value are 
pointless.

Does this opEquals do anything different from the default one? As far as 
I see, `e is this` does the same as `value == e.value` and the default 
equality would do the same, too.

>
>      auto opBinary(string op)(typeof(this) e)
>      {
>          auto x = typeof(this)();
>          mixin("x.value = this.value "~op~" e.value;");
>          return x;
>      }
>
>      auto opUnary(string op)()
>      {
>          auto x = typeof(this)();
>          mixin("x.value = "~op~"this.value;");
>          return x;
>      }
>
>      auto opCast()
>      {
>          return value;
>      }

When is this opCast going to be called?

>
>      auto toString()
>      {
>          string s;
>          foreach(i, e; EnumMembers!E)
>          {
>              if (((1 << i) & value) == 1 << i)
>                  s ~= to!string(e) ~ " | ";
>          }
>          if (s.length > 0)
>              s = s[0..$-3];
>          return s;
>      }
>
>
>      this(string from)
>      {
>          char uOp = ' ';
>          char Op = '=';

Convention says: Don't capitalize variable names. So it should be "op" 
instead of "Op".

>          char nOp = '=';
>          int previdx = 0;
>          value = 0;
>          for(int i = 0; i < from.length; i++)

There's a subtle problem here with i's type. from.length is a size_t, 
i.e. ulong in a 64 bit program. So it may be larger than int.max. When 
that happens, you've an infinite loop here, as i can never reach 
from.length. Solution: use size_t for i and previdx.

Also, foreach is nicer:

     foreach (i; 0 .. from.length)

>          {
>
>              nOp = from[i];
>              if (nOp == '!' || nOp == '~')
>              {
>                  uOp = nOp;
>                  previdx = i + 1;
>                  continue;
>              }
>
>              if (nOp == '&' || nOp == '^' || nOp == '|' || nOp == '+' ||
> nOp == '-' || i == from.length-1)
>              {
>
>                  auto v = from[previdx..(i + ((i == from.length-1) ? 1 :
> 0))].strip();
>                  vtype x = 0;
>                  foreach(j, e; EnumMembers!E)
>                      if (to!string(e) == v)
>                          x = cast(vtype)(1 << j);

Here you're shifting by the enum member's index. In opDispatch you shift 
by its value.

That difference leads to this:

     enum E {a = 0, b = 2}
     alias F = EnumToFlags!E;
     assert(F("a") is F.a); /* passes */
     assert(F("b") is F.b); /* fails */

>
>                  if (uOp == '!')
>                      x = !x;
>                  if (uOp == '~')
>                      x = ~x;
>
>                  switch(Op)
>                  {
>                      case '&': value = cast(vtype)(value & x); break;
>                      case '|': value = cast(vtype)(value | x); break;
>                      case '+': value = cast(vtype)(value + x); break;
>                      case '-': value = cast(vtype)(value - x); break;
>                      case '^': value = cast(vtype)(value ^ x); break;
>                      case '=': value = cast(vtype)(x); break;
>                      default: assert("Invalid EnumFlags operation
> `"~Op~"`.");
>                  }
>
>                  previdx = i + 1;
>                  Op = nOp;
>                  uOp = ' ';
>              }
>          }
>
>      }
>
>      alias value this;
> };

No semicolon after struct declarations in D.

You forgot to include some tests or a usage example. It's not obvious to 
me what EnumToFlags is supposed to accomplish or how to use it.

>
> Possibly the to and from string stuff can be improved? Simply apply to a
> pre-existing enum.

I'm not sure what the purpose of all this is, but it seems a bit 
convoluted to me.

Conversion from string seems pointless. In my opinion, this:

     alias F = EnumToFlags!E;
     auto f = F("foo | bar");

isn't better than this:

     alias F = EnumToFlags!E;
     auto f = F.foo | F.bar;

For conversion to string I can't think of a use case other than 
debugging. And for debugging a free function would be ok, too, I think.

Without the conversions from/to string, the struct just forwards to the 
value member, and one could just use the integer directly.

Have you considered simply generating a new enum with power-of-two 
values? For example, like so:

     template Flags(E) if (is(E == enum))
     {
         import std.format: format;
         import std.traits: EnumMembers;

         enum code = () {
             string result = "enum Flags {";
             foreach (e; EnumMembers!E)
                 result ~= format("%s = 1L << %dL,", e, e);
             return result ~ "}";
         }();
         mixin(code);
     }

     unittest
     {
         enum E
         {
             e0, e1, e2, e3, e4, e5, e6, e7, e8, e9,
             e10, e11, e12, e13, e14, e15, e16, e17, e18, e19,
             e20, e21, e22, e23, e24, e25, e26, e27, e28, e29,
             e30, e31, e32, e33, e34, e35, e36, e37, e38, e39,
             e40, e41, e42, e43, e44, e45, e46, e47, e48, e49,
             e50, e51, e52, e53, e54, e55, e56, e57, e58, e59,
             e60, e61, e62, e63
         }

         with (Flags!E)
         {
             assert((e0 | e1) == 0b11);
             assert((e1 | e3) == 0b1010);
             assert((e0 | e63) == 0x80_00_00_00_00_00_00_01);
             assert((e0 | e9 | e18 | e27 | e36 | e45 | e54 | e63) ==
                 0x80_40_20_10_08_04_02_01);

             assert(0b1010 & e1);
             assert(0b1010 & e3);
             assert(!(0b1010 & e0));
             assert(!(0b1010 & e2));
             assert(!(0b1010 & e4));
         }
     }