mattbierner / hamt

Javascript Hash Array Mapped Trie
MIT License
251 stars 16 forks source link

Problem with updateCollisionList #2

Closed maciej-sz closed 10 years ago

maciej-sz commented 10 years ago

To my understanding Collision.children should contain only instances of Leaf. But the updateCollisionList function inserts to that array raw output of the f (in two places) which results in children containing raw values.

var updateCollisionList = \list f k -> {
    for (var i = 0, len = list.length; i < len; i = i + 1)
    with child = list.(i) in {
        if (child.key === k) {
            return let v = f(child.value) in    /* raw v */
                ?isNothing v
                    :arraySpliceOut(i, list)
                    :arrayUpdate(i, v, list);   /* insert to children */
        }
    }

    return let v = f() in                       /* raw v again */
        ?isNothing v
            :list
            :arrayUpdate(list.length, v, list); /* insert to children */
};

This sometimes causes undefined property (child.key) error while invoking Collision.lookup:

Collision.prototype.lookup = \_ h k =self-> {
    if (h === self.hash) {
        for (var i = 0, len = self.children.length; i < len; i = i + 1)
        with child = self.children.(i) in {
            if (k === child.key)  /*** error: child has no "key" property ***/
                return child.value;
        }
    }
    return nothing;
};
mattbierner commented 10 years ago

Thanks. Back port of logic from HAMT+ introduced this bug in V0.1.5 but no tests caught it. Should be fixed now:

var updateCollisionList = \h list f k -> {
    var target, i = 0;
    for (var len = list.length; i < len; i = i + 1)
    with child = list.(i) in {
        if (child.key === k) {
            target = child;
            break;
        }
    }

    return let v = ?target : f(target.value) : f() in
        ?isNothing v
            :arraySpliceOut(i, list)
            :arrayUpdate(i, new Leaf(h, k, v), list);
};