Pointers inside constructor

[mitrious]

I have a node class that has a pointer to Node indicating its parent Node

class Node
{
.
.
.
Node *parentNode;
};

I receive a pointer to node in the constructor being the parent Node of the current node being created or a NULL value if it's the first node ( I do other stuff in the constructor as well but they are not relevant here

Node(const Node* parentNode) : parentNode(0) //set parentNode to NULL
{
      if(parentNode != NULL) //if the parentNode is NULL we just leave it at that
      {
            //Otherwise we set this pointer to point to this node
            this->parentNode = new parentNode(parentNode->parentNode);
      }
}

The thing is when I do that, if I create a destructor that destroys the parentNode pointer my program won't work cause I'll be deleting the same memory space I will use later (for example if I pass the node to a function and it gets copied), my copy constructor looks like this:

Node(const Node& n) : parentNode(0)
{
        if(n.parentNode != NULL)
        {
            this->parentNode = new Node(n.parentNode->parentNode)
        }
}

Am I wrong about this ? Should I really NOT use a destructor or should I be using something different and then use a destructor.

[deceptikon]

If the constructor depends on a pointer then it should make a copy in all cases or in no cases. That way the class is guaranteed to always own the pointer and deleting it is safe, or the pointer is always assumed to be owned by an external entity and deleting it is not safe:

// Pointer always owned
class Node
{
public:
    Node(Node const* parent)
    {
        if (parent == nullptr)
        {
            throw runtime_error("Invalid pointer");
        }

        _parent = new Node(*parent);
    }

    ~Node()
    {
        delete _parent;
    }
private:
    Node* _parent;
};

When the class owns the pointer it's safer to use one of C++'s smart pointers instead of pairing up new and delete.

// Pointer always owned
class Node
{
public:
    Node(Node const* parent)
    {
        if (parent == nullptr)
        {
            throw runtime_error("Invalid pointer");
        }

        _parent.reset(new Node(*parent));
    }
private:
    unique_ptr<Node> _parent;
};

Using an aliased pointer isn't as clean cut as owning the pointer, but it works as long as the class never assumes the pointer is valid or can be deleted.

Am I wrong about this ?

I think so, but I'm making an assumption about your class. A node class usually just needs to store the pointer because the memory is managed by whatever data structure class is creating the node objects. It could be as simple as this, for a binary tree:

struct Node
{
    Node(Node* left, Node* right, Node* parent = nullptr): _left(left), _right(right), _parent(parent)
    {}

    Node* _left;
    Node* _right;
    Node* _parent;
};

A destructor isn't needed because there's nothing to destroy, and the constructor is only there for convenience in creating objects in one shot.

[mitrious]

If the constructor depends on a pointer then it should make a copy in all cases or in no cases. That way the class is guaranteed to always own the pointer and deleting it is safe, or the pointer is always assumed to be owned by an external entity and deleting it is not safe:

// Pointer always owned
class Node
{
public:
    Node(Node const* parent)
    {
        if (parent == nullptr)
        {
            throw runtime_error("Invalid pointer");
        }

        _parent = new Node(*parent);
    }

    ~Node()
    {
        delete _parent;
    }
private:
    Node* _parent;
};

When the class owns the pointer it's safer to use one of C++'s smart pointers instead of pairing up new and delete.

// Pointer always owned
class Node
{
public:
    Node(Node const* parent)
    {
        if (parent == nullptr)
        {
            throw runtime_error("Invalid pointer");
        }

        _parent.reset(new Node(*parent));
    }
private:
    unique_ptr<Node> _parent;
};

Using an aliased pointer isn't as clean cut as owning the pointer, but it works as long as the class never assumes the pointer is valid or can be deleted.

I think so, but I'm making an assumption about your class. A node class usually just needs to store the pointer because the memory is managed by whatever data structure class is creating the node objects. It could be as simple as this, for a binary tree:

struct Node
{
    Node(Node* left, Node* right, Node* parent = nullptr): _left(left), _right(right), _parent(parent)
    {}

    Node* _left;
    Node* _right;
    Node* _parent;
};

A destructor isn't needed because there's nothing to destroy, and the constructor is only there for convenience in creating objects in one shot.

I see, so I wasn't wrong, that's what I thought, that I wouldn't need a destructor, and thanks for the reply

[deceptikon]

I see, so I wasn't wrong, that's what I thought, that I wouldn't need a destructor, and thanks for the reply

If the code allocate memory with new, there needs to be a corresponding delete. So yes, if your constructor uses new, there should probably be a destructor that uses delete, and a copy constructor that appropriately does both.