HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.15k stars 656 forks source link

Wierd behaviour of EnumValueMap 4.0.0-rc.5 #8901

Open CrazyFlasher opened 4 years ago

CrazyFlasher commented 4 years ago
class Test {

    static function main() {
        var map:Map<EnumValue, Int> = new Map<EnumValue, Int>();

        var ev_1:EnumValue = A.o;
        var ev_2:EnumValue = B.o;

        map.set(ev_1, 1);
        map.set(ev_2, 2);

        trace(ev_1 == ev_2);
        trace(map.get(ev_1), map.get(ev_2));
        trace(map.get(ev_1) == map.get(ev_2));
    }
}

enum A
{
    o;
    z;
}

enum B
{
    o;
    z;
}
14:30:27:626   Test.hx:13: false
14:30:27:626   Test.hx:14: 2,2
14:30:27:626   Test.hx:15: true
Aurel300 commented 4 years ago

The implementation has a comparison that compares the indices of the enum values (regardless of their type), then the parameters (none in this case):

https://github.com/HaxeFoundation/haxe/blob/319fa77c849e788f3b255c35f74f2df1ca430d95/std/haxe/ds/EnumValueMap.hx#L32-L41

So the enums resolve to the same key and the map in fact only contains one key after the two set calls.

Why are you using it this way? Do you want the map to contain values of different enum types?

CrazyFlasher commented 4 years ago

Well. That's rather strange and wrong comparsion, I think. What I want is the key as EnumValue :) I guess, I should extend EnumValueMap and override comparsion method

CrazyFlasher commented 4 years ago

I have MessageDispatcher class, where I want to use EnumValue map instead of StringMap to have unique message to callback mappings: https://github.com/CrazyFlasher/domwires-haxe/blob/master/src/com/domwires/core/mvc/message/MessageDispatcher.hx

 public function addMessageListener(type:EnumValue, listener:IMessage -> Void):Void
    {
        if (_messageMap == null)
        {
            _messageMap = new Map<EnumValue, Array<IMessage -> Void>>();
        }

        var messageMapForType:Array<IMessage -> Void> = _messageMap.get(type);
        if (messageMapForType == null)
        {
            messageMapForType = [];
            //To avoid check in this case, if vector contains element
            messageMapForType.push(listener);

            _messageMap.set(type, messageMapForType);
        } else
        if (messageMapForType.indexOf(listener) == -1)
        {
            messageMapForType.push(listener);
        }
    }
CrazyFlasher commented 4 years ago

What do you think, is it ok if I do this way?

import haxe.ds.EnumValueMap;

class Test {

    static function main() {
        var map:MessageMap = new MessageMap();

        var ev_1:EnumValue = A.o;
        var ev_2:EnumValue = B.o;

        map.set(ev_1, 1);
        map.set(ev_2, 2);

        trace(ev_1, ev_2);
        trace(ev_1 == ev_2);
        trace(map.get(ev_1) == map.get(ev_2));
    }
}

enum A
{
    o;
    z;
}

enum B
{
    o;
    z;
}

class MessageMap extends EnumValueMap<EnumValue, Int>
{
    override function compare(k1:EnumValue, k2:EnumValue):Int
    {
        if (k1 == k2) return super.compare(k1, k2);

        return -1;
    }
}
CrazyFlasher commented 4 years ago

What is the best ways to create unique keys for map? What class type of keys to use?

back2dos commented 4 years ago

Hmmm. This actually does feel like a bug. Ideally Map<EnumValue, V> would just not compile.

Anyway, for you case, you probably want something like this:

Map<String, Map<EnumValue, IMessage->Void>>, where the key to the outer map is Type.getEnumName(Type.getEnum(type)). On JavaScript (and probably quite a few other platforms) you can also use an ObjectMap<Dynamic, IMessage->Void>.

CrazyFlasher commented 4 years ago
class StringMessageType_1
{
    public static inline var TYPE_1:String = "TYPE_1";
}

class StringMessageType_2
{
    public static inline var TYPE_1:String = "TYPE_1";
}
enum EnumMessageType_1
{
    TYPE_1;
}

enum EnumMessageType_2
{
    TYPE_1;
}

Map<String, Map<EnumValue, IMessage->Void>> is not cool, because: StringMessageType_1.TYPE_1 == StringMessageType_2.TYPE_1 With enums EnumMessageType_1.TYPE_1 != EnumMessageType_2.TYPE_1

Simn commented 4 years ago

Using EnumValue as a Map key is pretty dirty, but I suppose we'll have to deal with this by also checking the type.

CrazyFlasher commented 4 years ago

Why dirty? What would be the best alternative?

Aurel300 commented 4 years ago

You can probably extend EnumValueMap and override compare like so:

override function compare(k1:EnumValue, k2:EnumValue):Int {
  var t1 = Type.getEnumName(Type.getEnum(k1));
  var t2 = Type.getEnumName(Type.getEnum(k2));
  if (t1 != t2)
    return t1 < t2 ? -1 : 1;
  return super.compare(k1, k2);
}

(as a temporary fix)

As an aside, I wonder why the compare function in the stdlib is using EnumValue instead of K for the argument types. Is that to have getIndex and getParameters?

CrazyFlasher commented 4 years ago

@Aurel300, thanks, passed the tests and seems to work as expected