Destructor for Node

[siala]

Hello!

Just a small reminder that the class Node (in XCSP3TreeNode.h) needs a destructor to free the memory. This is needed because Node contains:

std::vector<Node *> parameters ;

Thanks in advance :)

[massimomorara]

I'm agree with siala.

Non only Node but also Tree that contains a pointer to Node (root) that points to allocated memory but, as far I understand, is never dellallocated.

For Node (include/XCSP3TreeNode.h) it seems to me that should be enough add a virtual destructor (virtual is important, otherwise we could have undefined behavior deleting an object of a derived class) as follows

    virtual ~Node() {
       for ( auto & param : parameters )
          delete param;
    }

For Tree (include/XCSP3Tree.h) a destructor is needed

    ~Tree() {
       delete root;
    }

but also a modification in canonize() because the line

 root =  root->canonize();

assign a new value to root without deallocating the pointer initially pointed.

So I propose to modify Tree::canonize() as follows

    void canonize() {
       auto newRoot = root->canonize();

       delete root;

       root = newRoot;
    }

With this modification, valgrind signal to me less memory lost.

[siala]

Exactly! Thanks for reminding the issue.

[massimomorara]

Forget what I've written.

Yes, remain the problem that the Node::parameters are not deallocated and than also the Tree::root isn't allocated.

But, by example, calling canonize() you get a Node that (potentially) reference nodes present also in the original Node. So deleting the original root (in Tree::canonize()) you delete also some Nodes presents in the newRoot tree.

This seems to me a typical example of a case where smart pointer (std::shared_pointers, in particular) are useful. But, unfortunately, Tree::root and Node::parameters are public, so you can't change them using smart pointers or you break the existing code depending from the existing parser.