I see a couple problems here, but to start with your first question; you have no error checking. It's entirely possible that you are trying to delete [] a NULL handle - which would give that error you describe. To fix it,
/* Do the same thing for delete[] p below */
if (NULL != pParticles)
delete [] pParticles;
Also, there's no verification that the allocation even worked:
pParticles = new Particle[pCount];
if (NULL != pParticles) {
/* Do the copy operations here... */
}
That's it for the error checking - rudimentary and basic, but should help find your problem. Sprinkle in a couple of assert() calls and you should be good to go...
Now, for the second question: I would use an std::vector<> instead. Of course, behind the scenes, its doing the same thing as you are currently doing (more or less) but your code is a lot easier to read and follow... Here's how I would rewrite your current function:
#include <vector>
/* Typedefs - not neccessary but easier to read */
typedef std::vector<Particle> PARTICLE_LIST;
typedef std::vector<Particle>::iterator PARTICLE_LIST_POS;
typedef std::vector<Particle>::reverse_iterator PARTICLE_LIST_RPOS;
PARTICLE_LIST Particle_List; /* Has to exist somewhere else... */
void makeParticle(int iStartX, int iStartY, int iVelX, int iVelY) {
/*
Particle class has to have a Default constructor (no parameters), a
Copy constructor and an Assignment operator. All of which the compiler
will add automatically if you don't - but the automatic adds may not
work for your purposes - in which case you'd have to write your own
*/
Particle new_part;
new_part.init( iStartX, iStartY, iVelX, iVelY );
Particle_List.push_back(new_part);
/* NOTE: You can replace pCount variable with direct call to Particle_List.count() */
pCount = Particle_List.count();
}
Finally, the last thing I see is this would be relatively slow because (as far as I can tell) each particle object is an instance not a pointer to an instance.
Here's why: When ever you try to copy one instance to another, you are unknowingly calling the object's constructor / destructor pair for each copy. Allocations and deallocations are slow by nature. You want to limit the number of times you do either (if at all possible), and the best way to do that would be to create the new particle as a pointer (new) - then you can just copy the address in memory from one-array to another w/o having to call the constructor / destructor for each particle every time you copy them.
With that in mind, here's how I would rewrite the std::vector form of the function (again, to alleviate the number of constructor / destructor calls)
#include <vector>
/* Typedefs - not neccessary but easier to read */
typedef std::vector<Particle*> PARTICLE_LIST;
typedef std::vector<Particle*>::iterator PARTICLE_LIST_POS;
typedef std::vector<Particle*>::reverse_iterator PARTICLE_LIST_RPOS;
PARTICLE_LIST Particle_List; /* Has to exist somewhere else... */
void makeParticle(int iStartX, int iStartY, int iVelX, int iVelY) {
/*
Particle class has to have a Default constructor (no parameters), a
Copy constructor and an Assignment operator. All of which the compiler
will add automatically if you don't - but the automatic adds may not
work for your purposes - in which case you'd have to write your own
*/
Particle *new_part = new Particle;
if (NULL != new_part) {
new_part->init( iStartX, iStartY, iVelX, iVelY );
Particle_List.push_back(new_part);
}
/* NOTE: You can replace pCount variable with direct call to Particle_List.count() */
pCount = Particle_List.count();
}
I hope this helps,
JTK
