C++Talk.NET Forum Index C++Talk.NET
C++ language newsgroups
 
Archives   FAQFAQ   SearchSearch   MemberlistMemberlist   UsergroupsUsergroups   RegisterRegister 
 ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 

how to delete vectors that contain pointers to user-defined
Goto page 1, 2  Next
 
Post new topic   Reply to topic    C++Talk.NET Forum Index -> C++ language (comp.lang.c++)
View previous topic :: View next topic  
Author Message
Guest
PostPosted: Fri Jun 15, 2012 2:51 am    Post subject: how to delete vectors that contain pointers to user-defined Reply with quote


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?
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?
Back to top
Pavel
Guest
PostPosted: Fri Jun 15, 2012 2:51 am    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote


dwightarmyofchampions (AT) hotmail (DOT) com wrote:
Quote:
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
Quote:
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:
Quote:

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?
Quote:



HTH,
Pavel
Back to top
Juha Nieminen
Guest
PostPosted: Fri Jun 15, 2012 5:47 am    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote


dwightarmyofchampions (AT) hotmail (DOT) com wrote:
Quote:
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.
Back to top
Guest
PostPosted: Fri Jun 15, 2012 7:56 am    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
On Friday, June 15, 2012 4:51:03 AM UTC+2, (unknown) wrote:
Quote:
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 goes out of scope, it will delete the new Movie it holds. If push_back does not throw, 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;
}


Quote:
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?

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.

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

Yes.

Quote:
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).

Quote:
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 sole owner 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, given what we know so far.

Goran.
Back to top
Juha Nieminen
Guest
PostPosted: Fri Jun 15, 2012 9:52 am    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
goran.pusic (AT) gmail (DOT) com wrote:
Quote:
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>?
Back to top
Jorgen Grahn
Guest
PostPosted: Fri Jun 15, 2012 7:06 pm    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
On Fri, 2012-06-15, Tobias Müller wrote:
Quote:
Juha Nieminen <nospam (AT) thanks (DOT) invalid> wrote:
goran.pusic (AT) gmail (DOT) com 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 .
Back to top
Tobias Müller
Guest
PostPosted: Fri Jun 15, 2012 8:26 pm    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
Juha Nieminen <nospam (AT) thanks (DOT) invalid> wrote:
Quote:
goran.pusic (AT) gmail (DOT) com 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
Back to top
Tobias Müller
Guest
PostPosted: Sat Jun 16, 2012 12:15 pm    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
Jorgen Grahn <grahn+nntp (AT) snipabacken (DOT) se> wrote:
Quote:
On Fri, 2012-06-15, Tobias Müller wrote:
Juha Nieminen <nospam (AT) thanks (DOT) invalid> wrote:
goran.pusic (AT) gmail (DOT) com 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
Back to top
Guest
PostPosted: Sat Jun 16, 2012 4:56 pm    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
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.
Back to top
Guest
PostPosted: Sat Jun 16, 2012 5:36 pm    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
On Saturday, June 16, 2012 6:56:05 PM UTC+2, (unknown) wrote:
Quote:
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.
Back to top
Jorgen Grahn
Guest
PostPosted: Sat Jun 16, 2012 6:33 pm    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
On Sat, 2012-06-16, dwightarmyofchampions (AT) hotmail (DOT) com wrote:
Quote:
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.

Quote:
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 <grahn@ Oo o. . .
\X/ snipabacken.se> O o .
Back to top
Jorgen Grahn
Guest
PostPosted: Sat Jun 16, 2012 7:10 pm    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
On Sat, 2012-06-16, Tobias Müller wrote:
Quote:
Jorgen Grahn <grahn+nntp (AT) snipabacken (DOT) se> wrote:
On Fri, 2012-06-15, Tobias Müller wrote:
Juha Nieminen <nospam (AT) thanks (DOT) invalid> wrote:
goran.pusic (AT) gmail (DOT) com 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.

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.

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

--
// Jorgen Grahn <grahn@ Oo o. . .
\X/ snipabacken.se> O o .
Back to top
Guest
PostPosted: Sat Jun 16, 2012 8:13 pm    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
On Saturday, June 16, 2012 1:36:30 PM UTC-4, (unknown) wrote:
Quote:
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.


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.
Back to top
Guest
PostPosted: Sun Jun 17, 2012 8:02 am    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
On Saturday, June 16, 2012 10:13:22 PM UTC+2, (unknown) wrote:
Quote:
On Saturday, June 16, 2012 1:36:30 PM UTC-4, (unknown) wrote:
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.


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.

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 standard C++ 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 therefore should work in your case. In no way are you required to do it.
Back to top
Juha Nieminen
Guest
PostPosted: Mon Jun 18, 2012 6:58 am    Post subject: Re: how to delete vectors that contain pointers to user-defi Reply with quote
Jorgen Grahn <grahn+nntp (AT) snipabacken (DOT) se> wrote:
Quote:
(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.
Back to top
Post new topic   Reply to topic    C++Talk.NET Forum Index -> C++ language (comp.lang.c++) All times are GMT
Goto page 1, 2  Next
Page 1 of 2

 
  


Powered by phpBB © 2001, 2006 phpBB Group