Pointer not printing next value

[shaefayejem]

I am not sure, what is wrong with my code..
I was expecting the code to print

Child :: 1
Child :: 2

But it is only printing
Child :: 1

  #include <iostream>
class Child
{
private:
    int m_id;

public:
public:
    Child(int id) : m_id{id}
    {
    }
    void display()
    {
        std::cout << "Child :: " << m_id << std::endl;
    }
};

class Parent
{
private:
    Child *m_child;

public:
    Parent() : m_child(nullptr){};
    void setChild(Child *child)
    {
        if (this->m_child != nullptr)
            delete this->m_child;

        this->m_child = child;
    }
    void displayChildValues()
    {
        (*this->m_child).display();
    }
};

int main()
{
    Child child1{1};
    Child child2{2};

    Parent parent;
    parent.setChild(&child1);
    parent.displayChildValues();

    parent.setChild(&child2);
    parent.displayChildValues();
    return 0;
}

Not sure if I understood pointers correctly. Can somebody help please?

Thanks

[dhayden]

The call to Parent::setChild() at line 47 will cause the program to try to delete child1. Since child1 was not allocated on the heap, the behavior is undefined and the program goes off the rails at that point.

Anytime you're dealing with pointers, it's crucial to define who owns the pointer. The is the thing that's responsible for deleting it. In your case, if Parent owns the pointer then you need to pass in an object that was allocated on the heap:
parent.setChild(new Child{1}); // parent takes ownership of the new Child

[Peter87]

owns the pointer

You mean 'owns the object' (that the pointer points to)

[shaefayejem]

Thanks for the tip, so the fix is to edit the code and make it like this

Parent parent;
parent.setChild(new Child{1});
parent.displayChildValues();

parent.setChild(new Child{2});
parent.displayChildValues();

I am trying to wrap around the concept so kindly help me reinforce my understanding with follow up questions please. (Kudos!)

The call to Parent::setChild() at line 47 will cause the program to try to delete child1

After deleting child1, I am re-assigning it to the address of Child2. I am a bit confused in here. Isn't m_child be re-assigned to a new memory address of Child2?

this->m_child = child;

Also, how is the Parent taking ownership of the new Child with a call to "new Child{}"

I am a bit confused about the heap concept also so I need to do some reading on this and why understanding this is crucial in using pointers.

Thanks!

[shaefayejem]

After some more reading about the "new" and "delete" and the "heap" and the "stack", I think my problem is that I am deleting a pointer that is not created using the "new" keyword.

Local variables are created automatically on the stack while objects created with the new keyword is created on the heap.
So deleting a pointer not created by using the "new" keyword has some repercussions.
Is my understanding correct? Thanks.

[Ganado]

Your understanding is correct (but if I'm being pedantic, you're not deleting the pointer itself; it's more like you're freeing/"deleting" the data that is pointed to by the pointer).

PS:

        if (this->m_child != nullptr)
            delete this->m_child;

The null check here is unnecessary. Calling delete on a null pointer is a no-op.

PPS:
I want to reiterate what dhayden said: You must establish a clear ownership of the pointer (i.e. who is responsible for deleting it).

<edit: grammar>

[shaefayejem]

The null check here is unnecessary. Calling delete a null pointer is a no-op.

Sorry, I often see this "no-op" term while reading. Does it mean that even if the address that this pointer is pointing in the heap is null or with data, then it gets deleted? So that is why there is no need for null checking?

Thank you!

[Ganado]

By "no op" I mean "no operation", as in: nothing is done. No state is changed.

Calling delete nullptr; will not change the state of the program, so checking if a pointer is not null before deleting is redundant.

[shaefayejem]

Calling delete nullptr; will not change the state of the program, so checking if a pointer is not null before deleting is redundant.

This really makes sense to me right now.

Thank you all again!

[shaefayejem]

PPS:
I want to reiterate what dhayden said: You must establish a clear ownership of the pointer (i.e. who is responsible for deleting it).

I just saw your edit and I reviewed my code. Does it mean that I should add a delete on my destructor when the Parent goes out of scope just to reclaim the memory. Or this is not needed?

    Parent() : m_child(nullptr){};
    ~Parent()
    {
        delete this->m_child;
    }

[Ganado]

Right, that's good to have. (Actually, I didn't even notice that you didn't have a destructor. :p)

What I was saying was more about something to think about as you design your classes/program, and not a particular rule. There are wrong answers (e.g. deleting something with automatic storage), but there is not always one single, right answer. Passing a pointer from the outside into your class, from a point of syntax alone, is ambiguous, because it doesn't establish who now owns that pointer, and the memory it points to. So something extra is needed, such as external knowledge from documentation. Some libraries have attempted to reduce this ambiguity through concepts such as the Guideline Support Library's gsl::owner<T*> [I personally have only seen it used in talks; but just putting that out there if you want to search it up on your own.]

Your design isn't wrong, it's just that the user of your class needs to be sure they don't do something like

{
    Parent parent;
    Child child{1};
    parent.setChild(&child);
}

Libraries like wxWidgets do something similar - they expect items to be allocated with new in order to do proper memory cleanup when a child widget is destroyed (the user doesn't usually call delete themselves).

An alternative that might work, or may not, depending on the use cases of your classes, is to avoid having the end user create the child objects/pointers directly to begin with. For example, you might have something like the following, where all the children are owned by the parent, and the user only indirectly creates a Child by calling "create_child". Again, just brainstorming ideas; I'm not saying "do this".

class Parent
{
    void create_child(int id)
    {
        children.push_back(Child{id});
    }

    void set_active_child(int id)
    {
        for (Child& child : children)
        {
            if (child.id == id)
            {
                active_child = &child;
                break;
            }
        }
    }

    vector<Child> children;
    Child* active_child = nullptr;
};

[shaefayejem]

Thanks. This is great stuff and something that I have not been seeing in the documentation or tutorials online.

Kudos!