how to delete vectors that contain pointers to user-defined type objects

[dwightarmyofchampions]

Hello. I have a couple questions about vectors of pointers. Here is my code:


#include <iostream>
#include <vector>

class Movie
{
public:
Movie(std::string title, int year);
~Movie();

private:
std::string title;
int year;
};

class Libraries
{
public:
Libraries(std::vector<Movie*> rhs_library);
~Libraries();

private:
std::vector<Movie*> video_library;
};


Movie::Movie(std::string rhs_title,
int rhs_year)
{
title = rhs_title;
year = rhs_year;
return;
}

Movie::~Movie()
{
return;
}

Libraries::Libraries(std::vector<Movie*> rhs)
{
this->video_library = rhs;
return;
}

Libraries::~Libraries()
{
return;
}

int main()
{
Libraries* MyLibrary = 0;
std::vector<Movie*> movies;

for (int i = 0; i < 10; ++i)
{
movies.push_back(new Movie("A Movie", 1979));
}

//vector of movies is built up, so add it to MyLibrary
MyLibrary = new Libraries(movies);

// ...

for (std::vector<Movie*>::iterator it = movies.begin();
it != movies.end();
it++)
{
delete *it;
}

delete MyLibrary;

std::cout << "END" << std::endl;
return 0;
}


So, movies is a vector containing 10 objects of type Movie*.

1. When we push_back a new Movie*, we are copying by value, so the new Movie parameter immediately goes out of scope. Is that a memory leak, and if sohow do I fix it?
2. To delete the ten elements in movies, do I leave the iterator for loop where it is or do I put it in the Libraries destructor, or do both?
3. Are movies and this->video_library pointing to the same memory? I guess that would be so, since that's what the line in the constructor is doing, pointing them both to the same address in memory. If so, how do I be sure that I'm deleting the elements being pointed to only once? If I change the Libraries destructor to the following:

Libraries::~Libraries()
{
for (std::vector<Movie*>::iterator it = this->video_library.begin();
it != this->video_library.end();
it++)
{
delete *it;
}
return;
}


…I get a segmentation fault. How do I fix this? Isn't there an if statement I can use or something?

 

[Pavel]

Hello. I have a couple questions about vectors of pointers. Here is my code:


#include <iostream>
#include <vector>

class Movie
{
public:
Movie(std::string title, int year);
~Movie();

private:
std::string title;
int year;
};

class Libraries
{
public:
Libraries(std::vector<Movie*> rhs_library);
~Libraries();

private:
std::vector<Movie*> video_library;
};


Movie::Movie(std::string rhs_title,
int rhs_year)
{
title = rhs_title;
year = rhs_year;
return;
}

Movie::~Movie()
{
return;
}

Libraries::Libraries(std::vector<Movie*> rhs)
{
this->video_library = rhs;
return;
}

Libraries::~Libraries()
{
return;
}

int main()
{
Libraries* MyLibrary = 0;
std::vector<Movie*> movies;

for (int i = 0; i < 10; ++i)
{
movies.push_back(new Movie("A Movie", 1979));
}

//vector of movies is built up, so add it to MyLibrary
MyLibrary = new Libraries(movies);

// ...

for (std::vector<Movie*>::iterator it = movies.begin();
it != movies.end();
it++)
{
delete *it;
}

delete MyLibrary;

std::cout << "END" << std::endl;
return 0;
}


So, movies is a vector containing 10 objects of type Movie*.

1. When we push_back a new Movie*, we are copying by value, so the new Movie parameter immediately goes out of scope. Is that a memory leak,

No. You are not copying Movie but only a pointer. There is no memory leak

2. To delete the ten elements in movies, do I leave the iterator for loop where it is or do I put it in the Libraries destructor, or do both?

either one, but not both. destructor is better as you only write it once as
opposed to the loop in main() that has to be written for every use of Libraries

3. Are movies and this->video_library pointing to the same memory?

Yes
I guess that would be so, since that's what the line in the constructor is
doing, pointing them both to the same address in memory. If so, how do I be sure
that I'm deleting the elements being pointed to only once? If I change the
Libraries destructor to the following:

Libraries::~Libraries()
{
for (std::vector<Movie*>::iterator it = this->video_library.begin();
it != this->video_library.end();
it++)
{
delete *it;
}
return;
}


…I get a segmentation fault. How do I fix this?

Remove your deleting loop from main() if you delete in destructor (which is
usually a better choice).
Isn't there an if statement I can use or something?

HTH,
Pavel

 

[Juha Nieminen]

Libraries* MyLibrary = 0;
std::vector<Movie*> movies;

for (int i = 0; i < 10; ++i)
{
movies.push_back(new Movie("A Movie", 1979));
}

Is there a reason why you are not making a vector of Movie objects (rather
than a vector of pointers)? In other words:

std::vector<Movie> movies;

for(int i = 0; i < 10; ++i)
movies.push_back(Movie("A Movie", 1979));

Libraries myLibrary(movies);

This way you don't have to worry if those objects will be freed.

 

[goran.pusic]

Hello. I have a couple questions about vectors of pointers. Here is my code:


#include <iostream>
#include <vector>

class Movie
{
public:
Movie(std::string title, int year);
~Movie();

private:
std::string title;
int year;
};

class Libraries
{
public:
Libraries(std::vector<Movie*> rhs_library);
~Libraries();

private:
std::vector<Movie*> video_library;
};


Movie::Movie(std::string rhs_title,
int rhs_year)
{
title = rhs_title;
year = rhs_year;
return;
}

Movie::~Movie()
{
return;
}

Libraries::Libraries(std::vector<Movie*> rhs)
{
this->video_library = rhs;
return;
}

Libraries::~Libraries()
{
return;
}

int main()
{
Libraries* MyLibrary = 0;
std::vector<Movie*> movies;

for (int i = 0; i < 10; ++i)
{
movies.push_back(new Movie("A Movie", 1979));
}

//vector of movies is built up, so add it to MyLibrary
MyLibrary = new Libraries(movies);

// ...

for (std::vector<Movie*>::iterator it = movies.begin();
it != movies.end();
it++)
{
delete *it;
}

delete MyLibrary;

std::cout << "END" << std::endl;
return 0;
}


So, movies is a vector containing 10 objects of type Movie*.

1. When we push_back a new Movie*, we are copying by value, so the new Movie parameter immediately goes out of scope. Is that a memory leak, and if so how do I fix it?

It is a memory leak only if push_back throws an exception, which it can do.If it doesn't, then your "new Movie" will be deleted in "delete *in" line.

To avoid a (potential) memory leak, use e.g. unique_ptr:

unique_ptr<Movie> p(new Movie(...));
movies.push_back(p.get());
p.release();

Why is this correct? Because unique_ptr takes ownership of the pointer and it's constructor is guaranteed not to throw. What do I mean by "ownership"?I mean, when "p" is destroyed, it will delete the object it holds (if any).. Now... If push_back throws, p.release will not be called, and when p goesout of scope, it will delete the new Movie it holds. If push_back does notthrow, then p.release will be called, which causes "p" to stop holding a new Movie. Note that code above is a more compact way of writing following:

Movie* p = new Movie(...);
try { movies.push_back(p); }
catch(...)
{
delete p;
throw;
}

2. To delete the ten elements in movies, do I leave the iterator for loopwhere it is or do I put it in the Libraries destructor, or do both?

Both: no, that's impossible. That would mean that you are deleting each Movie twice, and that is undefined behavior. Answer to your question is: who, in your mind, "owns" Movie objects on the heap? If it's Libraries object, then do what you tried in 3 and don't delete in main. Otherwise, what you have so far is OK.

All this discussion leads to the CRUCIAL question when designing C++ code with heap objects: for each object on the heap, code must be absolutely certain who "owns" the object (in the sense of "who is responsible of deleting it"), at ANY GIVEN POINT IN THE PROGRAM.

3. Are movies and this->video_library pointing to the same memory?
Yes.

I guess that would be so, since that's what the line in the constructor is doing, pointing them both to the same address in memory. If so, how do I be sure that I'm deleting the elements being pointed to only once?

No way out. Your code has to do it (see above).

If I change the Libraries destructor to the following:

Libraries::~Libraries()
{
for (std::vector<Movie*>::iterator it = this->video_library.begin();
it != this->video_library.end();
it++)
{
delete *it;
}
return;
}


…I get a segmentation fault. How do I fix this? Isn't there an if statement I can use or something?

This deletes Movie instances twice, and that's a bug. You can do it only once, either in main or in ~Libraries.

A solution to your problem could be to use shared_ptr:

std::vector< std::shared_ptr<Movie> >;

This is because shared_ptr is designed exactly to handle shared ownership of heap objects. In your case, ownership is shared between your main and MyLibrary.

However, in your case, I would err towards making Libraries object the soleowner of Movie instances. That is, I would have e.g. Libraries::Add(Movie*m) and ~Libraries you have shown. That seems to be the simplest way, givenwhat we know so far.

Goran.

 

[Juha Nieminen]

unique_ptr<Movie> p(new Movie(...));
movies.push_back(p.get());
p.release();

Movie* p = new Movie(...);
try { movies.push_back(p); }
catch(...)
{
delete p;
throw;
}

std::vector< std::shared_ptr<Movie> >;

What is wrong with just std::vector<Movie>?

 

[Tobias Müller]

What is wrong with just std::vector<Movie>?

It is probably just a an example of a Problem that happened in a larger
context.
1. When Movie is a large object (e.g. contains the cover image) and has no
move constructor, copying is expensive.
2. There may other objects containing pointers to that movies. This is at
least problematic. You have to be sure, that the objects are never copied.
3. If there are subclasses of Movie you have to use pointers.

I mostly use pointers when identity is more important than equality. I
think this is usually called object semantics vs. value semantics.
I think identity is important, if the objects correspond to real physical
objects.

Example:
You have an additional class Person that contains Pointers to the Movies
that the person borrowed from you. If you have two copies of the same
movie, you may still want to know which one of them he borrowed exactly.

Tobi

 

[Jorgen Grahn]

It is probably just a an example of a Problem that happened in a larger
context.
1. When Movie is a large object (e.g. contains the cover image) and has no
move constructor, copying is expensive.
2. There may other objects containing pointers to that movies. This is at
least problematic. You have to be sure, that the objects are never copied.
3. If there are subclasses of Movie you have to use pointers.

Yes -- except the OP's very basic questions makes one wonder if one of
those was his reason, or if the vector<T*> design happened by accident
or misunderstanding.

/Jorgen

 

[Tobias Müller]

Yes -- except the OP's very basic questions makes one wonder if one of
those was his reason, or if the vector<T*> design happened by accident
or misunderstanding.

/Jorgen

Even if that's the case, it leads him to the wrong direction.
A std::vector<std::shared_ptr<Movie>> almost equally safe with respect to
memory leaks.
The problems you can run into with a std::vector<Movie> however can be much
more subtle and difficult to even notice them. Consider an insert or a
push_front into the vector. In the most cases all pointers to the existing
objects are still valid but now pointing to different objects.

Tobi

 

[dwightarmyofchampions]

This must be pointer because it's a VCL class, so it must be declared using new on the heap.

It is also an old compiler so I can't use fancy new pointers like shared_ptr or auto_ptr.

 

[goran.pusic]

This must be pointer because it's a VCL class, so it must be declared using new on the heap.

It is also an old compiler so I can't use fancy new pointers like shared_ptr or auto_ptr.

I don't remember what was available in VCL at what point, nor do you specify the compiler version, but I would be surprised to hear that auto_ptr is not available in it, as auto_ptr is pretty old.

By the way, if your context is VCL, you might find TComponent interesting. It allows ownership handling (see http://docwiki.embarcadero.com/Libraries/en/System.Classes.TComponent#Description, "Ownership" paragraph). Basically, you would derive Movie and Libraries from TComponent, and create your movies e.g. like this:

// Shorten typing
typedef auto_ptr<Libraries> PLibraries;
typedef auto_ptr<Movie> PMovie;

PLibraries l(new Libraries(...));
PMovie m(new Movie(...));
m.Owner = l.get(); // From now on, l "owns" m. No deleting necessary anymore
m.release();

Downside is that Components property doesn't give you Movie*, but a TComponent*. You might decide to expose Movies property in Libraries, e.g:

class Libraries
{
__property Movie* Movies[int Index] =
{read = { return static_cast<Movie*>(Components[index]) }};
};

(you have to make sure that you only ever "add" Movie instances to Libraries for the above to work).
Goran.

 

[dwightarmyofchampions]

I don't remember what was available in VCL at what point, nor do you specify the compiler version, but I would be surprised to hear that auto_ptr is not available in it, as auto_ptr is pretty old.

Specifically, it is C++ Builder in Borland Developer Studio 2006. I don't think you can use boost or auto_ptrs with that, but I might be wrong.

 

[Jorgen Grahn]

This must be pointer because it's a VCL class, so it must be declared using new on the heap.

"Visual Component Library", some Delphi thing. I had to look it up.

It is also an old compiler so I can't use fancy new pointers like shared_ptr or auto_ptr.

Uh, hasn't auto_ptr been standard for 15 years or so? (Not that it
helps here since AFAIK it doesn't mix well with containers.)

/Jorgen

 

[Jorgen Grahn]

Even if that's the case, it leads him to the wrong direction.

No, the default strategy for everyone should be not to mess with
pointers, plain or smart. It's things like 1--3 above which may force
you to do it.

A std::vector<std::shared_ptr<Movie>> almost equally safe with respect to
memory leaks.
The problems you can run into with a std::vector<Movie> however can be much
more subtle and difficult to even notice them. Consider an insert or a
push_front into the vector. In the most cases all pointers to the existing
objects are still valid but now pointing to different objects.

But everyone has to learn to handle that anyway! Unless they /never/
store objects by value in containers, and that would look rather
unusual.

(Of course, this being comp.lang.c++, someone will now claim
containers of pointers is the only way to do it, and be very
insistent.)

/Jorgen

 

[goran.pusic]

Specifically, it is C++ Builder in Borland Developer Studio 2006. I don'tthink you can use boost or auto_ptrs with that, but I might be wrong.

You can most certainly do what I have shown you about setting an Owner for your TComponent. In fact, if you want your code to be exception-safe, that's exactly what you need to do - that's idiomatic C++, VCL or not. Alternatively, you should use the try/catch from my first post. Naive code, e.g.

TWhateverComponent* p = new TWhateverComponent(...)
p->Owner = someOtherTComponent;

is not exception-safe.

I don't quite understand why you think that you can't use boost or standardC++ lib with VCL. They don't mix (how could they? VCL is written in another language altogether), that is true, but they won't cause each other to stop working either.

Goran.

P.S. I speak of TComponent::Owner and TComponent::Components only because this is an out-of-the box way to create ownership trees in VCL and thereforeshould work in your case. In no way are you required to do it.

 

[Juha Nieminen]

(Of course, this being comp.lang.c++, someone will now claim
containers of pointers is the only way to do it, and be very
insistent.)

Storing pointers to individually allocated objects is necessary in some
situations (mainly those where you need runtime polymorphism) but in
practice this is quite rarely the case. You probably need to do so more
often in certain types of applications or libraries (a GUI library being
a good example), but even then it's relatively rare.

Someone raised the concern that copying large objects inside a vector can
be expensive. If that's a real concern, then std::deque is the solution to
that. (It's not like std::vector is the only available data container.)
IMO better to use std::deque than going the complicated route of starting
to allocate individual objects and handling pointers to them. That's only
going to end in trouble.

 

[Old Wolf]

1. When we push_back a new Movie*, we are copying by value, so the new Movie parameter immediately goes out of scope. Is that a memory leak, and if so how do I fix it?

You copy the pointer by value; so you have two pointers to the same
memory (temoprarily). Then one pointer goes out of scope (but the
thing it's pointed to is still alive), no problem.

2. To delete the ten elements in movies, do I leave the iterator for loopwhere it is or do I put it in the Libraries destructor, or do both?

Well, if it's in both then you get a segfault because you called
'delete' twice on the same pointer. My answer would be 'neither'
though, see below

3. Are movies and this->video_library pointing to the same memory? I guess that would be so, since that's what the line in the constructor is doing,pointing them both to the same address in memory. If so, how do I be sure that I'm deleting the elements being pointed to only once? If I change the Libraries destructor to the following:

That line in the constructor is creating a second vector
that's an exact copy of the first. You now have two vectors
each containing 10 pointers (and the pointers in one
point to the same memory as the pointers in the other).

This is OK per se, but you have to think carefully about how
is responsible for deleting the pointers.

Of course, boost::shared_ptr is one option; another is using
the VCL ownership mechanism, as other posters have suggested.

If you didn't like either of those, what you could do is put
all your "new"'d stuff into a container, which you then
"delete" at the end. For example:

vector<Movie *> all_movies;
Movie *temp = new Movie(....);
all_movies.push_back(temp);
movies.push_back(temp);

and then manipulate your movies with impunity, but don't tough
"all_movies". Then at the end of your program delete everything
in all_movies.

This setup doesn't work so well if you want to delete
movies before the end of the program; shared_ptr is
designed exactly for that.

 

[Guest]

[rather than using some sort of pointer]

<snip>

[reasons to use containers of pointers]

No, the default strategy for everyone should be not to mess with
pointers, plain or smart. It's things like 1--3 above which may force
you to do it.


But everyone has to learn to handle that anyway! Unless they /never/
store objects by value in containers, and that would look rather
unusual.

this is one of the reasons I prefer pointers...

(Of course, this being comp.lang.c++, someone will now claim
containers of pointers is the only way to do it, and be very
insistent.)

well, I'm not going to be very insistant, but. Maybe it's my C/embedded back-ground but as soon as the object becomes non-trivial in size then I automatically think "pointers" (I grew up when memory were very small and copying *anything* was expensive). Plus the polymorphism thing. Am I unusual, that containers of polymorphic objects are very common in my designs?