Deleting new object passed to member function in C++

Question

just trying to wrap my head around how I go about deallocating memory of new objects which are passed as arguments.

Say I have a Link class defined like this:

class Link{
public:
    Link(const std::string& value, Link* previous = nullptr, Link* successor = nullptr) : _value{ value }, _previous{ previous }, _successor{ successor }{}

    Link* Insert(Link* new_link);

    Link* getPreviousLink() const{ return _previous; }
    Link* getSuccessorLink() const{ return _successor; }

    std::string _value;
private:
    Link* _previous;
    Link* _successor;
};

And then I have Link* Insert(Link*) defined like so:

Link* Link::Insert(Link* new_link){
    if(!new_link){ return this; }
    if(!this){ return new_link; }
    new_link->_successor = this;
    if(_previous){ _previous->_successor = new_link; }
    new_link->_previous = _previous;
    _previous = new_link;
    return new_link;
}

And then in my main(), I do the following:

int main(){
    Link* Cars = new Link("Ford");
    Cars = Cars->Insert(new Link("Ferrari"));
    Cars = Cars->Insert(new Link("Hummer"));
    Cars = Cars->Insert(new Link("Volvo"));
}

I've created a Link pointer called 'Cars' and allocated a new Link on the heap with a value of "Ford". I then assign my Cars pointer to a new Link returned from Insert(). Repeat this step 2 more times.

My question is, how do I delete or free the memory allocated when I pass the new Link objects as arguments? Do I do so in the destructor of Link? If I just delete my Cars pointer, it wouldn't deallocate the other Links.

`if(!this){ return new_link; }` means you invoked a method on a null pointer. Not a good idea. Not sure if it's flat-out Undefined Behaviour, but it happening is usually a sign of something in your code that requires a re-think. — user4581301, May 15 '18 at 00:17
`if(!this){ return new_link; }`is suspicious as it is UB to call a member function when `this` is nul. — Phil1970, May 15 '18 at 00:17
If you're going to use `new`, you must decide which piece of code is responsible for deleting that data structure. Yes, you can do it in `~Link()`, but then you must be sure you never call that destructor on a Link that has a static Link as a successor. — Beta, May 15 '18 at 00:20
@user4581301 see https://stackoverflow.com/questions/2474018/when-does-invoking-a-member-function-on-a-null-instance-result-in-undefined-behav/2474021 — Mark Ransom, May 15 '18 at 00:21
@MarkRansom Thank you. Reading my way through https://stackoverflow.com/questions/28482809/c-access-static-members-using-null-pointer . Will read that next. Never mind, what I'm reading does not apply. — user4581301, May 15 '18 at 00:24
@Beta Thanks for the reply, I'm actually following along in a book and found it weird that they never included a destructor. In my own implementation I've added a destructor which deletes _previous and _successor but I thought I'd ask just to be sure :) — xIIPANIKIIx, May 15 '18 at 00:35
@user4581301 I'm actually following along with a book. I kind of guessed that when I was writing it as it doesn't make much sense. So is that line of code useless considering it'll never be reached if I tried called it on a nullptr? — xIIPANIKIIx, May 15 '18 at 00:40
It deletes `_previous` *and* `_successor`? I hope you're being careful with the pointers. — Beta, May 15 '18 at 00:43
@Beta yeah, isn't that what I should be doing? In my use-case, if I deleted one, I'd want all other Links gone. And delete can be called on a nullptr so it doesn't really matter right? I'm only learning pointers so this is all new to me. — xIIPANIKIIx, May 15 '18 at 00:48
That test doesn't make much sense in C++ with a class, but it makes perfectly good sense in C. The author of your book might know their C a bit better than their C++ or there might be some secret plan they'll get to later. There isn't much better for Intro to Pointers than a linked list, but in real-world coding, you'll either do something along the lines of what Jarod42 has in his answer using smart pointers or keep the individual links real stupid and keep all of the smarts in a Linked List class. Even more likely you'll just use `std::list` or `std::vector`. — user4581301, May 15 '18 at 00:57
The danger of deleting `_previous` and `_successor` is the odds are really good that you got to the current node by deleting it from `_previous`'s destructor, and deleting the same object twice does not go over well. — user4581301, May 15 '18 at 01:00
Think about it. A->B, you try to delete A, A's dtor tries to delete B, B's dtor tries to delete A, A's (second) dtor tries to delete B; in principle this goes on forever, in practice after a few more rounds you overload the call stack and crash. I'm all in favor of implementing a doubly link list in order to learn pointercraft, but that means learning to spot these problems. — Beta, May 15 '18 at 01:12

score 4 · Accepted Answer · answered May 15 '18 at 00:28

With smart pointer, ownership would be clear:

class Link : public std::enable_shared_from_this<Link> {
public:
    Link(const std::string& value) : _value{ value } {}

    std::shared_ptr<Link> Insert(std::shared_ptr<Link> new_link);

    std::shared_ptr<Link> getPreviousLink() const{ return _previous.lock(); }
    std::shared_ptr<Link> getSuccessorLink() const{ return _successor; }

    std::string _value;
private:
    std::weak_ptr<Link> _previous;
    std::shared_ptr<Link> _successor;
};

std::shared_ptr<Link> Link::Insert(std::shared_ptr<Link> new_link)
{
    if (!new_link){ return shared_from_this(); }
    new_link->_successor = shared_from_this();
    auto prev = _previous.lock();
    if (prev) { prev->_successor = new_link; }
    new_link->_previous = prev;
    _previous = new_link;
    return new_link;
}

int main(){
    auto Cars = std::make_shared<Link>("Ford");
    Cars = Cars->Insert(std::make_shared<Link>("Ferrari"));
    Cars = Cars->Insert(std::make_shared<Link>("Hummer"));
    Cars = Cars->Insert(std::make_shared<Link>("Volvo"));
}

Deleting new object passed to member function in C++

1 Answers1