ag0aep6g 
| 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));
}
}
|
Permalink
Reply
