0

I have a set of classes to handle Disjoint sets in my C++ app. I have a hard time implementing the destructors for these classes. Can anyone help me on that?

What these basically do is: put nodes' pointers into NodeAddress[], every node is distinguished by its val. Every node has a pointer to the Item which is a placeholder for hd head and tl tail of the disjoint set.

I want to mention that I realize that there are some issues with e.g. : variable visibility ( public access ), constant size NodeAddress buffer but I want to focus here on memory deallocation.

And yes I want (need) to do it on pointer (no STL). If you have sny suggestions or questions feel free to comment.

This is the code :

header

class ListSet {
public:
    unsigned int size;
    node* NodeAddress[MAX_NUMBER_OF_LABELS];

    struct Item;
    class node {
    public:
        unsigned int val;
        node *next;
        Item *itemPtr;

        node () : val(0), next(0), itemPtr(0) {}
        node (const int& a) : val(a), next(0), itemPtr(0) {}
    };
    struct Item {
    public:
        node *hd, *tl;
        Item(node *shd) : hd(shd), tl(shd) {}
        void ListSet::Item::append (const Item* other);

        //removal
        ListSet::node* remove(node* old);
    };

    ListSet()
    {
        this->size = 0;
        memset(NodeAddress, 0, sizeof(NodeAddress));
    }

    void setNodeAddress(const int& a, node* shd)
    {
        NodeAddress[a] = shd;
    }
    node* getNodeAddress(const int& a)
    {
        return NodeAddress[a];
    }

    ListSet::Item* ListSet::makeSet (const int& a) ;
    ListSet::Item* ListSet::find (const int& a);

    ListSet::Item* ListSet::unionSets (Item* s1, Item* s2);
    void ListSet::unionSets (const int& a1, const int& a2);
};

source

void ListSet::Item::append (const Item* other) {
    //join the tail of the set to head of the other set
    tl->next = other->hd;
    tl = other->tl;
    for (node* cur = other->hd; cur; cur = cur->next) {
        cur->itemPtr = this;
    }
}

ListSet::Item* ListSet::makeSet (const int& a) {
    if( a > this->size) {this->size = a;}

    assert(!getNodeAddress(a));
    node *shd = new node(a);
    Item *newSet = new Item(shd);
    setNodeAddress(a, shd);
    shd->itemPtr = newSet;
    return newSet;
}

ListSet::Item* ListSet::find (const int& a) {
    node* ptr = getNodeAddress(a);
    if (ptr)
        return ptr->itemPtr;
    return 0;
}

ListSet::Item* ListSet::unionSets (Item* s1, Item* s2) {
    Item *set1 = s1;
    Item *set2 = s2;

    set2->append(set1);
    delete set1;

    return set2;
}
void ListSet::unionSets (const int& a1, const int& a2) {
    Item* s1 = find(a1);
    Item* s2 = find(a2);
    if (s1 && s2) {
        (void) unionSets(s1, s2);
    }
}

*EDIT: * there is something that I have but is not working

ListSet::node* ListSet::Item::remove(node* old) {
    if (old == hd) {
        if (old == tl) {
            assert(! old->next);
            return 0;
        }
        assert(old->next);
        hd = old->next;
    } else {
        node* prev;
        for (prev = hd; prev->next != old; prev = prev->next) {
            assert(prev->next);
            ;
        }
        if (old == tl) {
            assert(! old->next);
            //
            tl = prev;
            prev->next = 0;
        } else {
            assert(old->next);
            prev->next = old->next;
        }
    }
    return hd;
}

ListSet::node::~node() {
    if (itemPtr) {
        if (! itemPtr->remove(this)) {
            // Don't leak an empty set.
            delete itemPtr;
        }
    }
}

void ListSet::remove(const int& a) {
    node* ptr = getNodeAddress(a);
    if (ptr) {
        setNodeAddress(a, 0);
        delete ptr;
    }
    // else error?
}
Patryk
  • 22,602
  • 44
  • 128
  • 244
  • 2
    Why do you "want/need" to "do it on pointer"? The crown skill of a good programmer is the ability to break a problem down into recognizable components and use established, high-quality building blocks to solve each constituent part and assemble them into a solution. Every line of code that you *don't* write is a mistake you didn't make. – Kerrek SB Feb 27 '12 at 20:12
  • 1
    @KerrekSB My guess is it is homework. – Stephan van den Heuvel Feb 27 '12 at 20:15
  • @KerrekSB Because I am writting an app for Windows Mobile and I encountered a problem with `STL` - as soon as I add the `STL` header to the project I get the Pinvoke. I couldn't resolve it normally so I have gone for pointers. – Patryk Feb 27 '12 at 20:17
  • @KerrekSB That's such a great quote. I hope you don't mind if I borrow it sometimes. – stinky472 Feb 27 '12 at 20:46
  • @Patryk yeah STL on windows mobile is a dog.. Though I didn't see any suggestion you should use it. – baash05 Feb 28 '12 at 02:02
  • how about you include your destructor code. – baash05 Feb 28 '12 at 02:22
  • @baash05 I have just a sample but it is not working as intended. [http://pastebin.com/n5Emd1gJ](http://pastebin.com/n5Emd1gJ) – Patryk Feb 28 '12 at 03:35
  • @Patryk the biggest problem I see is that the data is managed in a circle. classes should not typically have circular references to memory. I would go with a STL free vector. – baash05 Feb 28 '12 at 05:39
  • I don't see any of the objects, really holding anything. Where (beyond pointers to themselves.) is the data? – baash05 Feb 28 '12 at 05:43
  • @baash05 Actually the only reference to the data is in the `NodeAddress` array. I think this should be somehow improved but I am not really sure how. – Patryk Feb 28 '12 at 19:47

1 Answers1

1

Your code is overly complicated. Here's my take on a disjoint set forest; all memory management can be handled from the outside by putting the sets in a vector. Note that no pointer manipulation is needed, because a set can be uniquely identified by a size_t-typed index into the forest.

Fred Foo
  • 355,277
  • 75
  • 744
  • 836
  • Ok.. I agree with this.. even without a the STL vector.. Make your own vector class with a simple linked list, or if you need to grab things by index, create a grow-able array.. should be easy as.. – baash05 Feb 28 '12 at 05:38