Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C++ > how to delete vectors that contain pointers to user-defined type objects

Reply
Thread Tools

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

 
 
dwightarmyofchampions@hotmail.com
Guest
Posts: n/a
 
      06-15-2012
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?
 
Reply With Quote
 
 
 
 
Pavel
Guest
Posts: n/a
 
      06-15-2012
http://www.velocityreviews.com/forums/(E-Mail Removed) wrote:
> 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
 
Reply With Quote
 
 
 
 
Juha Nieminen
Guest
Posts: n/a
 
      06-15-2012
(E-Mail Removed) wrote:
> 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.
 
Reply With Quote
 
goran.pusic@gmail.com
Guest
Posts: n/a
 
      06-15-2012
On Friday, June 15, 2012 4:51:03 AM UTC+2, (unknown) wrote:
> 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.
 
Reply With Quote
 
Juha Nieminen
Guest
Posts: n/a
 
      06-15-2012
(E-Mail Removed) wrote:
> 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>?
 
Reply With Quote
 
Tobias Müller
Guest
Posts: n/a
 
      06-15-2012
Juha Nieminen <(E-Mail Removed)> wrote:
> (E-Mail Removed) wrote:
>> 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>?


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
 
Reply With Quote
 
Jorgen Grahn
Guest
Posts: n/a
 
      06-15-2012
On Fri, 2012-06-15, Tobias Müller wrote:
> Juha Nieminen <(E-Mail Removed)> wrote:
>> (E-Mail Removed) wrote:
>>> 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>?

>
> 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

--
// Jorgen Grahn <grahn@ Oo o. . .
\X/ snipabacken.se> O o .
 
Reply With Quote
 
Tobias Müller
Guest
Posts: n/a
 
      06-16-2012
Jorgen Grahn <(E-Mail Removed)> wrote:
> On Fri, 2012-06-15, Tobias Müller wrote:
>> Juha Nieminen <(E-Mail Removed)> wrote:
>>> (E-Mail Removed) wrote:
>>>> 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>?

>>
>> 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


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
 
Reply With Quote
 
dwightarmyofchampions@hotmail.com
Guest
Posts: n/a
 
      06-16-2012
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.
 
Reply With Quote
 
goran.pusic@gmail.com
Guest
Posts: n/a
 
      06-16-2012
On Saturday, June 16, 2012 6:56:05 PM UTC+2, (unknown) wrote:
> 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/Libra...nt#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.
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Re: How include a large array? Edward A. Falk C Programming 1 04-04-2013 08:07 PM
problem in running a basic code in python 3.3.0 that includes HTML file Satabdi Mukherjee Python 1 04-04-2013 07:48 PM
RCR: String#contain? and Array#contain? Roger Pack Ruby 3 09-28-2010 04:13 PM
Does string contain A, and if so, does a section of string contain B Jason Carlton Javascript 11 12-08-2009 06:07 PM
Deleting elements of vectors that contain pointers to other objects dwightarmyofchampions@hotmail.com C++ 7 03-20-2009 09:06 AM



Advertisments