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,=20
             int rhs_year)
{
	title =3D rhs_title;
	year =3D rhs_year;
	return;
}

Movie::~Movie()
{
	return;
}

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

Libraries::~Libraries()
{
	return;
}

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

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

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

	// ...

	for (std::vector<Movie*>::iterator it =3D movies.begin();
	     it !=3D 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*.=20

1. When we push_back a new Movie*, we are copying by value, so the new Movi=
e parameter immediately goes out of scope. Is that a memory leak, and if so=
 how do I fix it?  =20
2. To delete the ten elements in movies, do I leave the iterator for loop w=
here 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, p=
ointing them both to the same address in memory. If so, how do I be sure th=
at I'm deleting the elements being pointed to only once? If I change the Li=
braries destructor to the following:

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


=85I get a segmentation fault. How do I fix this? Isn't there an if stateme=
nt I can use or something?

dwightarmyofchampions@hotmail.com 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

dwightarmyofchampions@hotmail.com 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.

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 co=
de:
>=20
>=20
> #include <iostream>
> #include <vector>
>=20
> class Movie
> {
> 	public:
> 		Movie(std::string title, int year);
> 		~Movie();
>=20
> 	private:
> 		std::string title;
> 		int year;
> };
>=20
> class Libraries
> {
> 	public:
> 		Libraries(std::vector<Movie*> rhs_library);
> 		~Libraries();
>=20
> 	private:
> 		std::vector<Movie*> video_library;
> };
>=20
>=20
> Movie::Movie(std::string rhs_title,=20
>              int rhs_year)
> {
> 	title =3D rhs_title;
> 	year =3D rhs_year;
> 	return;
> }
>=20
> Movie::~Movie()
> {
> 	return;
> }
>=20
> Libraries::Libraries(std::vector<Movie*> rhs)=20
> {
> 	this->video_library =3D rhs;
> 	return;
> }
>=20
> Libraries::~Libraries()
> {
> 	return;
> }
>=20
> int main()
> {
> 	Libraries* MyLibrary =3D 0;
> 	std::vector<Movie*> movies;
>=20
> 	for (int i =3D 0; i < 10; ++i)
> 	{
> 		movies.push_back(new Movie("A Movie", 1979));
> 	}
>=20
> 	//vector of movies is built up, so add it to MyLibrary
> 	MyLibrary =3D new Libraries(movies);
>=20
> 	// ...
>=20
> 	for (std::vector<Movie*>::iterator it =3D movies.begin();
> 	     it !=3D movies.end();
> 	     it++)
> 	{
> 		delete *it;
> 	}
>=20
> 	delete MyLibrary;
>=20
> 	std::cout << "END" << std::endl;
> 	return 0;
> }
>=20
>=20
> So, movies is a vector containing 10 objects of type Movie*.=20
>=20
> 1. When we push_back a new Movie*, we are copying by value, so the new Mo=
vie parameter immediately goes out of scope. Is that a memory leak, and if =
so how do I fix it?  =20

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 n=
ew Movie. Note that code above is a more compact way of writing following:

Movie* p =3D 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 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 Mov=
ie 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, t=
hen do what you tried in 3 and don't delete in main. Otherwise, what you ha=
ve so far is OK.

All this discussion leads to the CRUCIAL question when designing C++ code w=
ith heap objects: for each object on the heap, code must be absolutely cert=
ain 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?=20

Yes.

> I guess that would be so, since that's what the line in the constructor i=
s 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?=20

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

> If I change the Libraries destructor to the following:
>=20
> Libraries::~Libraries()
> {
> 	for (std::vector<Movie*>::iterator it =3D this->video_library.begin();
> 	     it !=3D this->video_library.end();
> 	     it++)
> 	{
> 		delete *it;
> 	}
> 	return;
> }
>=20
>=20
> =85I get a segmentation fault. How do I fix this? Isn't there an if state=
ment I can use or something?

This deletes Movie instances twice, and that's a bug. You can do it only on=
ce, 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 o=
f heap objects. In your case, ownership is shared between your main and MyL=
ibrary.=20

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.

goran.pusic@gmail.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>?

On Jun 15, 2:51=A0pm, dwightarmyofchampi...@hotmail.com wrote:
> 1. When we push_back a new Movie*, we are copying by value, so the new Mo=
vie 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 loop=
 where 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 gues=
s 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 =3D 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.