Deleting a vector of unique_ptr

[alexbcg]

Hi, I am currently working on a project for my programming class which involves simulating a system of particles and their interactions.
I have created a System class, which includes the Particle class (and the other classes which have inherited from Particle, eg Neon/Argon/etc...)

In order to store the contents of the system (ie the particles), I have created a vector of unique pointers that point onto the aforementioned Particles.
We have been asked to add a method to the System class that can delete all of the particles in the system (ie make the vector of Particles empty).

I was therefore wondering what the best approach was, as I am aware that unique_ptr is supposed to take care of memory management but being very wary of pointers I'm not sure what to do in order to keep everything nice and clean?

Finally, am I correct in assuming that one does not need to explicitly write a destructor since the minimal version generated by the compiler should suffice thanks to the unique_ptr ?

P.S. I cannot post my actual code because although we are encouraged to ask for advice, code sharing can in some cases be considered cheating so sorry about that

Many Thanks :)

[Peter87]

Simply call clear to make the vector empty.
http://en.cppreference.com/w/cpp/container/vector/clear

You don't need to add anything to the System class' destructor to handle the particle vector.

[alexbcg]

Thanks that's great :)
However I have noticed I am having what seems to be some kind of memory leak when calling the addParticle method I have implemented in the System class. Everything compiles fine and it does what it is supposed to do when it runs but at the end of the output I get

Projet(36886,0x7fff71f13310) malloc: *** error for object 0x7fff5fbff8b0: pointer being freed was not allocated
Program ended with exit code: 9

The code I am trying is as follows (implemented in the System.cpp file)

void System::addParticle(Particle &part)
{
    std::unique_ptr<Particle> newParticle(&part);
    collectionParticules.push_back(std::move(newParticle));
}

I know something is very wrong but I don't know what and searching forume hasn't helped :/

Additionally, the Particle class is abstract so when I tried

collectionParticules.push_back(std::unique_ptr<Particle>(new Particle(part)));

I get the error

Allocating an object of abstract class Particle

I don't understand this because in the main I will want to pass objects that are of the type of the classes (Neon, Argon etc have redefined all the =0 methods so they are not abstract) that have inherited from the abstract class Particle. It would seem silly to write a function that has to explicit the derived types and not just the super class because objects of type Neon are Particles...

[LB]

You're misusing polymorphism and pointers. How are you even calling your addParticle function? The way this is set up, you would have to do something absurd like this:

addParticle(*(new Neon(blah, blah));

Instead, consider something like this (untested):

template<typename ParticleT, typename... Args>
void System::addParticle(Args &&... args)
{
    collectionParticules.emplace_back(
        std::unique_ptr<Particle>(
            new ParticleT(std::forward(args)...) ));
}

addParticle<Neon>(blah, blah);

[Peter87]

You should only store elements created with new with std::unique_ptr (or write your own deleter but then you better know what you are doing and for what purpose).

#include <memory>

int main()
{
	std::unique_ptr<int> p;
	{
		int i = 0;
		p.reset(&i);
	} // <- i goes out of scope here.
	
	// If you try to use *p you will get undefined behaviour because 
	// i no longer exists.
	
} // p goes out of scope here so it will try to clean up the object by 
  // calling delete but that will not work because i no longer exists
  // and even if it did it would not work because delete should only 
  // be used on objects created with new. 

[alexbcg]

Thanks for your help guys,
Indeed I know I am misusing them but I just don't see what the correct approach is.
The addParticle method is supposed to take as an argument an object of the type Neon/Argon/Helium (which is a Particle as it is derived from the class Particle) and simple add a unique_ptr to vector<unique_ptr<Particle>> collectionParticules; (which is a private attribute of the System class).
This unique_ptr is supposed to point to the object that was passed as an argument to the addParticle method. The object passed as an argument continues to live on happily somewhere in memory and is subsequently used by accessing the members of the collectionParticules vector and using these pointers.
I am also having gto avoid copying objects unnecessarily so things don't get slow when the whole thing runs.
Really quite confused right now and I have a feeling it's something very straightforward :/

[LB]

You still have not given a concrete example of how you're calling addParticle.

[alexbcg]

NB:
The Vecteur class is included in the particle class, a particle has three private attributes, its' position and speed (which are of type Vecteur) and its' mass (a double)
The output() method simply outputs the contents of the system class (eg The system has n particles, prints out the contents of each particle as there exists an overloaded << operator in the Particle class that prints out the type of particle (He,Ne...), position, speed, masss)

int main()
{
Helium part1(Vecteur(1,2,3), Vecteur(4,5,6), 8);
Neon part2(Vecteur(1,5,3), Vecteur(3,4,5),2);
System system;
system.addParticle(part1);
system.addParticle(part2);
system.output();
return 0;
}

The output is fine with this code, the output() function even prints out what it's supposed to by iterating through the unique_ptrs in the vector and prints out the right types etc there is just a memory problem going on somewhere.
The code used to add a particle is the same as written in "The code I am trying is as follows (implemented in the System.cpp file)" in my post May 12, 2014 at 11:19am.

Once again sorry about not being able to post big chunks of code :/
By selectively commenting out lines, putting in breakpoints I have confirmed beyond a doubt that issue really lies with the addParticle method.

[coder777]

use unique_ptr only if you want to point once to an object. If you want it more than once use shared_ptr:

http://www.cplusplus.com/reference/memory/shared_ptr/

void System::addParticle(std::shared_ptr<Particle> part)
{
    collectionParticules.push_back(part);
}

int main()
{
std::shared_ptr<Particle> part1(new Helium(Vecteur(1,2,3), Vecteur(4,5,6), 8));
std::shared_ptr<Particle> part2(new Neon(Vecteur(1,5,3), Vecteur(3,4,5),2));
...
system.addParticle(part1);
system.addParticle(part2);
...
}

[alexbcg]

Thanks, is it not possible in any way for the addParticle method to just take a reference to a particle object as an argument? ie

addParticle(Particle &part)
{
...
}

So that you don't have to make the pointers yourself in the main?

Because right now it's halfway there since the output is correct and it is possible for methods from within the class to access the elements pointed to by the pointers in the vector. It's just this damn memory problem upon execution...

[coder777]

you can do this:

void System::addParticle(Particle &part)
{
    collectionParticules.push_back(&part); // This is very insecure (scope!)
}

if Particle does not have further inheritance/virtual functions you can simply use a copy

[alexbcg]

Thanks very much for all your help, I managed to get there in the end, it works well. I have done something similar to what coder777 suggested, except I left it as a vector of unique ptr and passed a regular pointer as an argument which solves things and does not do anything unconventional as far as the code goes :)
Many thanks!