Two pointers pointing to same memory via class copy constructor. How to detect when one deletes the object?

[RickBlacker]

I have a custom C++ class Person that defines a few functions and data members. In my main app, I have something like this....

Person* fred	= new Person( "Fred" );
Person* barny   = fred;

delete barny;
delete fred;

Everything is fine until I try to delete fred which makes sense. Since barny is pointing to fred, and I call delete on barny, the original fred memory gets freed up... So, I thought I could do something like this:

if( fred )
   delete fred;

That didn't work so I tried:

if( fred != nullptr )
   delete fred;

That didn't work either. What is the proper way to ensure that fred is not null so that I can delete him? I realize in this very brief example it seems intuitive to just not try and delete fred, however in a real world example where you may have pointers pointing to the same object but in vast different areas of the code base, it may not be so intuitive.

Thanks!

[Ganado]

if (fred) and if (fred != nullptr) do the same thing.
So that at least isn't the issue.

So the problem is that both barney and fred point to the same person. I assume your code is just an example, because otherwise it makes no sense to have a pointer called barney point to a Person named Fred...

When delete is called on a pointer, the pointer does not automatically become null. After delete is called on a pointer, it should be treated as if it's an uninitialized variable. So you have to explicitly set it to null.

That's a neat fact, but sadly doesn't really help your problem. The fact is that, as your current example stands, there's no way for fred to know that the data it was pointing to was deleted. Because fred's value is just an address. It doesn't change when the data barney is pointing to is deleted.

You've stumbled upon the problem of who will delete the shared memory? You need to know which pointer will have a longer lifetime, and have that pointer's data be deleted, or you'd have to keep track of some sort of third object that knows both objects.

In the real world, you use modern C++ constructs like shared_ptrs, and not have to delete things yourself, because that can get hairy real quick.
http://en.cppreference.com/w/cpp/memory/shared_ptr
http://www.cplusplus.com/reference/memory/shared_ptr/shared_ptr/

[RickBlacker]

Hi Ganado...

Yes, this is just an example. I'm trying to dust off my C++ skills after years and years of programming in other languages.


This situation came up because I was testing a copy constructor. The goal was to do a deep copy of "p" into "this". I THINK, I've done it properly, but not 100% especially with the _name string.

Person::Person( const Person& p )
	: _name{ p.GetName() }, _spouse{ p.GetSpouse( ) }
{
	CopyKids( p.GetChildren( ) );
}

void Person::CopyKids( const vector<Person*>& srcVctr )
{
	if( srcVctr.size( ) > 0 )
	{
		for( unsigned x = 0; x < srcVctr.size( ); x++ )
		{
			Person* p = new Person( *srcVctr[ x ] );
			_children.push_back( p );
		}
	}
}

From the header file

string			_name;
Person*			_spouse;
vector<Person*>	_children;

Also was trying to test two operator overloads that I've setup... The '=' and '==' operators.

[Ganado]

Is there a particular need to have it be an std::vector<Person*> instead of an std::vector<Person>? (Oh, I see, you'd need the wife to point to the same data, so that gets complicated)

But your copy constructor looks fine to me, as long as you don't have infinite descendants :p
Or like some weird time paradox where you set yourself to be your own grandfather.

The if( srcVctr.size( ) > 0 ) part is redundant, because it will already skip the for loop is size is 0.

Maybe this page will help refresh things? http://www.cplusplus.com/articles/y8hv0pDG/

The == operator will have be similar to how you did things with the copying.

_____________________________________

However, if I may, it seems like the complexity of the operation will explode once you grow out your family tree. Do you really need to compare every single descendant to see if Person A == Person B? I feel like a better system might involve a unique ID for each added member, that could be incremented internally with a static member of the class.

[RickBlacker]

Hi Ganado...
Great question about

vector<Person> 
vector<Person*>

I guess it comes down to memory allocation. As I understand it (and I could be WAY incorrect). If I create an instance of a person like this.

Person p1( "Rick" ) 

This creates p1 on the stack. Where as if I create a person like this

Person* p1 = new Person( "Rick" ) 

That code gets generated on the heap.

OR does that stack/heap only apply when passing objects to functions?
If it only applies to passing to functions, then yeah... I would rather not deal with pointers and just pass them as reference as needed.

Again... I'm trying to dust of the thick cobwebs of my C++ skills from back in the mid 90's when I was in college. :D

[RickBlacker]

Just for grins.... and because this is just play code for my own purpose of learning... Here is the header and source file for my class..... Please feel free to give me any tips and heads up. :)

HEADER

#pragma once

#include <string>
#include <vector>

using namespace std;

class Person
{
public:
	Person( );
	Person( const std::string name );
	Person( const Person& p );
	Person( const Person& self, Person* spouse );
	Person( const Person& self, const Person* spouse, const vector<Person*>& children );

	~Person( );

	void			AddChild( Person* p );
	string			GetName( ) const;
	Person*			GetSpouse( ) const;
	void			AddSpouse( Person* p );
	vector<Person*>	GetChildren( ) const;

	string			ToString( );
	void			RemoveKids( );
	Person&			operator = ( const Person& p );
	bool			operator == ( const Person& p );

private:
	string			_name;
	Person*			_spouse;
	vector<Person*>	_children;

	void CopyKids( const vector<Person*>& srcVctr );
};

[RickBlacker]

SOURCE

#include "Person.h"
#include <string>
#include <iostream>

using namespace std;

//=============================================================================
// Constructor
//=============================================================================
Person::Person( )
	: _name{ "unknown" }, _spouse{ nullptr }
{ }


//=============================================================================
// Constructor
//=============================================================================
Person::Person( const std::string name )
	: _name{ name }, _spouse{ nullptr }
{ }


//=============================================================================
// Copy Constructor
//=============================================================================
Person::Person( const Person& p )
	: _name{ p.GetName() }, _spouse{ p.GetSpouse( ) }
{
	CopyKids( p.GetChildren( ) );
}


//=============================================================================
// Constructor
//=============================================================================
Person::Person( const Person& self, Person* spouse )
	: _name{ self.GetName() }, _spouse{ spouse }
{
	CopyKids( self.GetChildren( ) );
}


//=============================================================================
// Destructs the Person object freeing all resources in the children vector
//=============================================================================
Person::~Person( )
{ 
	cout << endl << "~DESTRUCTOR FOR : " << _name << endl;
	_name.clear( );
	if( _children.size() > 0 )
		RemoveKids( );
	_spouse		= nullptr;
}


//=============================================================================
// Returns the vector of children
//=============================================================================
//<remarks>blah blah blah</remarks>
vector<Person*> Person::GetChildren( ) const
{
	return _children;
}


//=============================================================================
// Returns the name of the Person
//=============================================================================
string Person::GetName( ) const
{
	return _name;
}


//=============================================================================
// Returns a pointer to this Person's spouse object
//=============================================================================
Person* Person::GetSpouse( ) const
{
	return _spouse;
}


//=============================================================================
// Adds a pre allocated Person to the list of children.
//=============================================================================
void Person::AddChild( Person* p )
{
	_children.push_back( p );
}


//=============================================================================
// Utility turns data into a string for displaying
//=============================================================================
string Person::ToString( )
{
	string s = "Name:\t\t" + _name;

	if( _spouse != nullptr )
		s += "\r\nSpouse:\t\t" + _spouse->GetName( ) + " ";
	
	int count = _children.size( );
	
	if( count > 0 )
	{
		for( unsigned x = 0; x < count; x++ )
			s += "\r\n  Child:\t" + _children[ x ]->GetName( ) + " " ;

		if( _spouse != nullptr &&  _spouse->GetChildren( ).size( ) > 0 )
		{
			vector<Person*> tmpV = _spouse->GetChildren( );
			for( unsigned y = 0; y < tmpV.size(); y++ )
				s += "\r\n  Child:\t" + tmpV[ y ]->GetName( ) + " ";
		}
	}
	return s;
}


//=============================================================================
// Assigns the spouse pointer
//=============================================================================
void Person::AddSpouse( Person* spouse )
{
	if( this->_spouse == nullptr )
		this->_spouse = spouse;
}


//=============================================================================
// Removes children from children vector, deallocates memory each child
// pointer pointed to.
//=============================================================================
void Person::RemoveKids( )
{
	int count = _children.size( );

	if( count <= 0 )
		return;
	
	for( int x = 0; x < count; x++ )
	{
		delete _children[ x ];
		_children[ x ] = nullptr;
	}
	_children.clear( );
}


//=============================================================================
// Performs a deep copy from srcVctr into this person's children vector.
//=============================================================================
void Person::CopyKids( const vector<Person*>& srcVctr )
{
	if( srcVctr.size( ) > 0 )
	{
		for( unsigned x = 0; x < srcVctr.size( ); x++ )
		{
			Person* p = new Person( *srcVctr[ x ] );
			_children.push_back( p );
		}
	}
}



//=============================================================================
// Performs a deep copy from srcVctr into this person's children vector.
//=============================================================================
Person& Person::operator = ( const Person& p )
{
	if( this != &p )
	{
		this->_name = p.GetName( );
		this->_spouse = p.GetSpouse( );
		CopyKids( p.GetChildren( ) );
	}
	return *this;
}



//=============================================================================
// Performs deep copy comparrison. 
//=============================================================================
bool Person::operator == ( const Person& p )
{
	bool bMatch = ( this->_name == p.GetName( ) && this->_spouse == p.GetSpouse( ) );
	
	if( bMatch )
	{
		bMatch = this->_children.size( ) == p.GetChildren( ).size( );

		if( bMatch )
		{
			if( this->_children.size( ) > 0 )
			{
				vector<Person*> tempV = p.GetChildren( );
				for( unsigned x = 0; x < tempV.size( ); x++ )
				{
					bMatch = ( this->_children[ x ] != tempV[ x ] );
					if( !bMatch )
						break;
				}
			}
		}
	}
	return bMatch;
}

[RickBlacker]

TEST APP

#include <iostream>
#include "Person.h"

using namespace std;

int main( )
{
	// Parents
	Person* fred	= new Person( "Fred" );
	Person* wilma	= new Person( "Wilma" );

	// Kids
	Person* pebbles = new Person( "Pebbles" );
	Person* rocky	= new Person( "Rocky" );
	Person* sage	= new Person( "Sage" );
	Person* kronk	= new Person( "Kronk" );
	Person* zed		= new Person( "Zed" );


	fred->AddChild( zed );
	fred->AddChild( kronk );

	wilma->AddChild( pebbles );
	wilma->AddChild( rocky );
	wilma->AddChild( sage );

	cout << endl << "=============== FRED SINGLE =============== " << endl;
	cout << fred->ToString( ) << endl;
	
	cout << endl << "=============== WILMA SINGLE =============== " << endl;
	cout << wilma->ToString( ) << endl;

	fred->AddSpouse( wilma );
	wilma->AddSpouse( fred );

	cout << endl << "=============== FRED MARRIED =============== " << endl;
	cout << fred->ToString( ) << endl;

	cout << endl << "=============== WILMA MARRIED =============== " << endl;
	cout << wilma->ToString( ) << endl;

	// ============================================================================================
	cout << endl << "=======================================" << endl;

	Person* barny = new Person( "Barny" );
	if( barny == fred )
		cout << " barny == fred" << endl;
	else
		cout << " barny != fred" << endl;

	delete barny;
	cout << endl << "=============== FRED AFTER BARNY =============== " << endl;
	cout << fred->ToString( ) << endl;


	barny = fred;

	if( barny == fred )
		cout << " barny == fred" << endl;
	else
		cout << " barny != fred" << endl;

	delete barny;

	if( fred != nullptr )
		delete fred;

	delete wilma;
	//delete pebbles;
	//delete rocky;
	//delete sage;
	//delete kronk;
	//delete zed;


	//Person* fred2 = new Person( *fred );

	//cout << "Fred1: " << fred->ToString( ) << endl;
	//cout << "Fred2: " << fred2->ToString( ) << endl;


	//cout << endl  << "==========================================" << endl;
	//cout << "Fred1: " << fred->ToString( ) << endl;
	//cout << "Fred2: " << fred2->ToString( ) << endl;

	//cout << endl << "==========================================" << endl;
	//fred->RemoveKids( );
	//cout << "Fred1: " << fred->ToString( ) << endl;
	//cout << "Fred2: " << fred2->ToString( ) << endl;


	char a;
	cin >> a;
}

[helios]

Objects contained in other objects are allocated wherever the outer object is allocated.

class A{
public:
    int a;
};

class B{
public:
     A a;
};

int main(){
    B b1;
    //b1.a is on the stack.
    B *b2 = new B;
    b2->a is on the heap.
}

[RickBlacker]

Hi helios

Right, so that's what I thought and as I had recently studied, just about everything but basic types should be "new"ed so that the memory gets allocated on the heap.

[Ganado]

Oh darn, I was busy and you posted a lot while I was gone.

But here are my thoughts

Yes, correct. The first example is on the stack, the second is on the heap. So if you need the data to outlive the scope its defined in, you'll need to use Person*'s.

I realized this could get super complicated, depending on the specifics what can happen. Might be a bit above my head to know the most efficient solution, but:

So, let's say we have it set up so that both Dad and Mom give birth to Son and Daughter. Dad has Mom set as spouse. Mom has dad set as Spouse. Dad and Mom both have a vector of 2x children pointers, and those point to Son and Daughter. Let's say something bad happens and Son dies, so you want to remove him from the (living) family tree. So, we need to decide who is in charge of the memory.

Let's say you have something like this

Person* dad;
Person* mom;
Person* son;
Person* daughter;

// ...allocate and set up relationships

TellParentsChildDied(dad, mom, son);

So now what? Once you tell mom and dad that their son died, they can each remove the son from their vector. But who is actually in charge of deleting the son's heap data (burying him?). We don't know.

One solution is to use shared_ptrs. You'd have to re-work your code, though. Might be complicated. I'm not an expert in this, although others in this forum are much more comfortable with this, hopefully they'll see the thread.

shared_ptr<Person> son = make_shared("Billy");

shared_ptrs do reference counting to ensure they don't delete heap-allocated memory that is still being referenced by someone else.

Once the last std::vector<std::shared_ptr<Person>> vector removes the son's shared_ptr, the destructor of the shared_ptr should know to decrement its reference count, and delete the actual data. RIP son.

But I suggest searching google for unique_ptr or shared_ptr guides, since I remember there being a ton of gotchas, but it's been a while since I've coded anything serious with them.

Person( const Person& self, Person* spouse );
What is this supposed to do? Copy self, and then re-assign the spouse? (Got a divorce :p) I think that's valid, but might seem like a confusing interface as opposed to just re-assigning the spouse.

________________________________

Edit: I also don't think you should have to deep copy or allocate anything just to compare two Persons (== operator).

[RickBlacker]

Yes, I agree, there is a big hole in this implementation with the person class having a vector of person representing kids. A better solution might be to have a person class, then a family class that had

class Family
{
public:
    Family( );
    Family( Person* mom, Person* dad, vector<Person*>kiddos );
private:
    Person* husband
    Person* wife
    vector<Person*> kids

    .......
}

I like the idea of looking into smart pointers. When I was in college there was no concept of smart pointers. Learning more about them is definitely on my TODO list.

Having said that, I wasn't really trying to implement a sound solution. Just playing with things and re familiarizing myself with C++ and the rules, best practices of the language itself.

[helios]

Watch out! You can't use smart pointers to represent circular data structures.

struct Node{
    std::shared_ptr<Node> next;
};

int main(){
    auto a = std::make_shared<Node>();
    auto b = std::make_shared<Node>();
    a->next = b;
    b->next = a;
    //At this point, the object graph contains a cycle and std::shared_ptr cannot release it properly.
}

Also, if none of the Persons in your Family class will be referenced by any other object, it makes more sense to make them simple objects, rather than pointers.

[RickBlacker]

That's good to know. Does the compiler complain? Does the app crash or do you just end up with a memory leak?

[helios]

No, the compiler cannot detect cycles in data structures.
Circular data structures with smart pointers cause a resource leaks (leaking an object can cause leakage of more than just memory).

[keskiverto]

All smart pointers are not equal. What about adding the std::weak_ptr into the mix?

[helios]

Well, weak_ptr isn't really a smart pointer, it's just a wrapper around a normal pointer. Either way, it doesn't help much in the case of a general graph.

[Repeater]

just about everything but basic types should be "new"ed so that the memory gets allocated on the heap.

Just the opposite. Use the heap only when you have to. The default choice should be stack. You need a good reason to use the heap.

[keskiverto]

True though, general graph is a different matter. Trees are easier.

IMHO a pointer that knows whether the object it points to has been deleted, is way above the plain, non-smart pointer.

Interestingly, http://en.cppreference.com/w/cpp/memory/weak_ptr writes:

In addition, std::weak_ptr is used to break circular references of std::shared_ptr.

and https://codesynthesis.com/~boris/blog//2010/05/24/smart-pointers-in-boost-tr1-cxx-x0/ wrote:

Boost shared_ptr is a shared pointer implementation that uses a separate reference counter allocated on the heap. weak_ptr is a companion pointer which points to the object owned by shared_ptr without having an increment in the reference counter. It is primarily useful to resolve cycles in an object ownership graph that would otherwise prevent the objects in the graph from ever being deleted.

The challenge is to know which edges of the graph must own, and which may not. A general graph should not have that distinction. All edges should be edges.

One could of course write a "smarter" pointer. Not trivial.


Plan B: One list of persons. Another list of relationships.

[Repeater]

One could of course write a "smarter" pointer. Not trivial.

Apropos of nothing much, I spent much of the last month tracking memory leaks in a decade old piece of software in which an early developer had proudly done just this, more or less. He put the reference counts inside the object being counted, so anything subject to this memory management was forced to inherit from a particular abstract class and implement some functions to have and manage this reference count, and then only use the objects with the particular kind of smart pointer he's made. Those function were implemented differently, badly, and in every single instance not thread safe at the point deletion of the object (which again was done by the object itself - delete this;), leading to objects with a count of one yet already deleted, or if it broke the other way objects to which no handles existed but with positive counts that would never be deleted. People have tried to manually fix the broken reference counting for themselves in little bits throughout, so any global fix would then lead to all those little fixes becoming bugs. Basically, I can rely on any given object used across threads to either leak, or cause a crash, depending on in which direction the reference counting is broken.

Because the objects themselves hold their own count, and do their own deletion, they must be access through the "clever" handle that knows about this; some objects managed to make their own copy constructors private, but some didn't, so years later someone comes along and copies an object or takes a pointer to it without knowing it's going to vanish without warning. The whole thing is a truly brutal mess, of which the original author (long since moved on) seems so oblivious that he namechecks the software product in his bio. He basically made every bad architectural choice and every mistake possible in implementing his own handles (i.e. smart pointers) and the bug DB has hundreds of cases where it went wrong in one direction (deleted too soon, crash when next used) or the other way (never deleted, leaks that the customer notices and sometimes even has to manually close and reopen the software to deal with).

I suppose what I'm doing is very violently agreeing with

Not trivial

, although usually the mistakes people make doing it are subtle and clever, and this guy was just a ham-fisted amateur with no awareness of his own limitations, no idea of threading, and a penchant for making horrifically complicated software through inheritance that people are forced to infect their good code with, spreading the contagion endlessly through the system. DO NOT WRITE YOUR OWN SMART POINTER :)