Better EnumFlags

[ncannasse]

Original haxe.EnumFlags come from a before-abstract Haxe, I think it would be time to revamp it.

Ideally one would like to be able to do the following:

@:enumFlags abstract Flags(Int) {
    var A; // = 1 automatically assigned
    var B; // = 2 
    var Special = 0x800;
    var None = 0; 
}
static function foo( f : Flags ) {
    if( f.has(A|Special) )
        f &= A;
   else
        f.set(B);
    ...
}
foo(None);
foo(A);
foo(A|Special);

Please note that this is currently not working well with current EnumFlags, because A and B are not EnumFlags instance but the enum type parameter, so the top town inference will not be able to correctly deal with A|B for example.

This is possible however to do by doing the following:

@:enum abstract Flags(Int) {

    var A = 1;
    var B = 2;
    var Special = 0x800;
    var None = 0; 

    @:op(a|b) static function or(a:Flags,b:Flags) : Flags;
    @:op(a&b) static function and(a:Flags,b:Flags) : Flags;

    public inline function new(i = 0) {
        this = i;
    }

    public inline function has( v : Flags ) : Bool {
        return (this & v.toInt()) == v.toInt();
    }

    public inline function set( v : Flags ) : Void {
        this |= v.toInt();
    }

    public inline function unset( v : Flags ) : Void {
        this &= 0xFFFFFFFF - v.toInt();
    }

    public inline function toInt() {
        return this;
    }

    public inline static function ofInt( i : Int ) : Flags {
        return new Flags(i);
    }   
}

It's quite verbose if you have to declare many of the and is not entirely correct either since it's not a proper "enum" in the sense that all flags combination as possible, so it's not something that should be considered complete with a switch.

I propose that we add @:enumFlags for that and deprecate haxe.EnumFlags. My only concern here is that if you want to have an "enum" that comes both in terms of different independent values AND that you want to gather as bit flags it doesn't work anymore.

Also being able to iterate on the individual flags and switch on it (this time with switch completeness) would be interesting, this could be done for instance by defining:

function iterator() : haxe.SingleFlag;

Another option is to keep the two types (the enum and the flags separated) but we still want the initial example to work well.

Any suggestion?

[Simn]

Note: EnumFlags as of the TEnumIndex-introduction are terrible because Type.enumIndex is not optimized properly anymore. I have disabled the related tests in https://github.com/HaxeFoundation/haxe/commit/de30c9cf0871b453426bbbf54f1a279a043a99d6.

Whatever solution we come up with here should cover this.

[nadako]

One solution could work is to make compiler-implemented EnumValueTools.getIndex() that produces TEnumIndex node (similar to how match is implemented) and then make Type.enumIndex force-inline calling that getIndex() function on all targets. One issue here is that if we do that right away, the getIndex() argument will be typed as EnumValue instead of original enum type, but I think that can be solved by making it parametrized like @:extern public static inline enumIndex<T:EnumValue>(e:T):Int return t.getIndex();

[Simn]

Yes that was my idea for it as well.

[ncannasse]

@simn uh ? you mean you have merged something that creates a big regression performance wise on something on the standard library without pushing the solution that comes with it ? if that's the case I vote for a full revert of the TEnumIndex until the problems that comes with it are properly addressed

[Simn]

I figured we could just solve this issue and move on. But whatever, I reverted the enumIndex optimization removal.

[nadako]

if that's the case I vote for a full revert of the TEnumIndex until the problems that comes with it are properly addressed

There was no regressions with TEnumIndex introduction itself (I checked the diffs, there were only improvements). It's just the Type.enumIndex call optimizations that were later removed (and now reverted until we come up with a better solution).

[ncannasse]

Ok got it, fine with me

On Mon, Jun 19, 2017 at 12:29 PM, Dan Korostelev notifications@github.com wrote:

if that's the case I vote for a full revert of the TEnumIndex until the problems that comes with it are properly addressed

There was no regressions with TEnumIndex introduction itself (I checked the diffs, there were only improvements). It's just the Type.enumIndex call optimizations that were later removed (and now reverted until we come up with a better solution).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HaxeFoundation/haxe/issues/6330#issuecomment-309401459, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-bwBI-NcrFl0oJA9K98ZzPMPIwJGWUks5sFk2dgaJpZM4NvWcR .

[nadako]

@markknol brought up this topic again today and that made me thinking:

Bitmask is a set of flags (so, basically a collection), so it is not an enum type: a set of values is not limited to the flags and exhaustiveness check does not make much sense for it.

Surely, there is an enumeration of possible flags which would be fine to represent as an enum abstract, but then we would need another type to represent a bitmask (a set of the flags), very similar to the current haxe.EnumFlags.

But since we're talking about brevity and convenience here, what we really want is an easy way to create handy and type-safe bitmask types. So I would propose something like:

@:bitmask abstract Flags(Int) {
  var A;
  var B;
  // ..etc
}

The compiler would then implement bit ops (and other proposed API) for this type as well as add support for unqualified access to the flags, so we can simply do A | B.

I'm not sure if there's a realistic case where we would want exhaustiveness check for these. Especially given that with bitmasks we often want to define variants which are combinations of other flags (or None = 0 in Nic's original example). So I don't think we should care about this here.

[Simn]

I was thinking of this today and read through the issue again. What confuses me about the original example is this part:

static function foo( f : Flags ) {
    if( f.has(A|Special) )
        f &= A;
   else
        f.set(B);
}
foo(None);

That looks like you want reference semantics, but if Flags is an abstract over Int that isn't going to work out. Is this just an oversimplified example or am I missing the point here?

As for the rest, I think that something like this already "just works":

abstract Flags<T:Int>(T) from T to T {
    public inline function new(value:T) {
        this = value;
    }

    @:op(a | b) function or(a:T):Flags<T>;

    @:op(a & b) function and(a:T):Flags<T>;

    public inline function has(v:T):Bool {
        return (this & v) == v;
    }

    public inline function set(v:T):Void {
        this = cast this | v;
    }

    public inline function unset(v:T):Void {
        this = cast this & (0xFFFFFFFF - v);
    }
}

enum abstract ActualFlags(Int) to Int from Int {
    var A = 1;
    var B = 2;
    var Special = 0x800;
    var None = 0;
}

I need unsafe casts in the implementation because Haxe infers T | T as Int for some reason, but that's a separate issue.

Edit: Forgot the actual usage:

function eq<T>(ex:T, act:T, ?p:PosInfos) {
    if (ex != act) {
        haxe.Log.trace('$act shoudld be $ex', p);
    }
}

function main() {
    var flags:Flags<ActualFlags> = new Flags(None);

    eq(false, flags.has(A));
    flags.set(A);
    eq(true, flags.has(A));
    flags.unset(A);
    eq(false, flags.has(A));

    eq(false, flags.has(B));
    flags |= B;
    eq(true, flags.has(B));
    flags |= A;
    eq(true, flags.has(B));
    eq(true, flags.has(A));
}

Generated JS:

function Main_main() {
    Main_eq(false,false,{ fileName : "source/Main.hx", lineNumber : 41, className : "_Main.Main_Fields_", methodName : "main"});
    Main_eq(true,true,{ fileName : "source/Main.hx", lineNumber : 43, className : "_Main.Main_Fields_", methodName : "main"});
    Main_eq(false,false,{ fileName : "source/Main.hx", lineNumber : 45, className : "_Main.Main_Fields_", methodName : "main"});
    Main_eq(false,false,{ fileName : "source/Main.hx", lineNumber : 47, className : "_Main.Main_Fields_", methodName : "main"});
    Main_eq(true,true,{ fileName : "source/Main.hx", lineNumber : 49, className : "_Main.Main_Fields_", methodName : "main"});
    Main_eq(true,true,{ fileName : "source/Main.hx", lineNumber : 51, className : "_Main.Main_Fields_", methodName : "main"});
    Main_eq(true,true,{ fileName : "source/Main.hx", lineNumber : 52, className : "_Main.Main_Fields_", methodName : "main"});
}

[onehundredfeet]

Did this go any further? Should I still be using EnumFlags as documented? https://api.haxe.org/haxe/EnumFlags.html

Will is be a performance hit over just using an int abstract?