CODEWITHSUNDEEP
c++
user3151683
user3151683

Reputation: 1

Storing references to a pointer

I need to create many instances of a class which I feel a vector would do as I need. I've read that a vector only stores a copy which isn't what I want as I need to be able to access and compute within the class. I've tried storing the pointer but it isn't working out either.

Tower.cpp

vector<Projectile*> projectiles;

void Tower::fire(Minion& m)
{
    projectiles.push_back(&Projectile(&m, x, 2, y));
}



void Tower::update(Level& level)
{
    for (int i = 0; i < projectiles.size(); i++)
    {
        projectiles.at(i)->update();
    }
}

Projectile.cpp

Minion* m;

void Projectile::update()
{
    float angle = atan((m->getX() - x) / (m->getY() - z));
    x += speed * sin(angle);
    z += speed * cos(angle);
}

Upvotes: 0

Views: 83

Answers (3)

ikh
ikh

Reputation: 10417

You are storing a pointer of LOCAL VARIABLES into global std::vector! It is TOO BAD!

Instead, I recommend to use container of std::shared_ptr. For example:

std::vector<std::shared_ptr<ProjectFile> > projectfiles;

void Tower::fire(Minion &m)
{
    projectfiles.push_back(std::make_shared<ProjectFile>(&m, x, 2, y));
}

If ProjectFile's constructor is not public, use std::shared_ptr's constructor instead of std::make_shared

projectfiles.push_back(std::shared_ptr<ProjectFile>(new ProjectFile(&m, x, 2, y)));

Upvotes: 1

Karl Nicoll
Karl Nicoll

Reputation: 16419

You can't do this:

projectiles.push_back(&Projectile(&m, x, 2, y));

What you're actually doing there is creating a variable of type Projectile, passing its address into the vector object, and then the Projectile instance is being destroyed because it goes out of scope. Once the Projectile instance has gone out of scope, the pointer will point to nothing.

What I suspect you're trying to do is this:

projectiles.push_back(new Projectile(&m, x, 2, y));

Do bear in mind however that it's still YOUR responsibility to destroy those Projectile objects when you're finished with them.

The following, for example, causes a memory leak:

projectiles.push_back(new Projectile(&m, x, 2, y));
projectiles.erase(0); // <-- Projectile object still exists, you've only deleted the pointer!!!

This would fix it:

projectiles.push_back(new Projectile(&m, x, 2, y));

// ... later ...
delete projectiles.at(0);
projectiles.erase(0);

Of course, the ideal way to do this would be to use a smart pointer like unique_ptr or shared_ptr to manage the deletion of your objects for you.

If you insist on using raw pointers, but are using Boost, you can also use the ptr_vector class to handle deletion for you as well.

Upvotes: 2

Lightness Races in Orbit
Lightness Races in Orbit

Reputation: 385174

Yikes; holy dangling pointers, batman!

projectiles.push_back(&Projectile(&m, x, 2, y));

You seem to be trying to store pointers to, not only local variables, but temporary objects! Not sure what you expect those pointers to refer to after that line has finished executing.

I don't see you storing references anywhere, though (which is good).

You're basically going to have to use dynamic allocation here so you can manage the lifetime of these objects yourself, though I'd suggest using a smart pointer type so that you don't have to take too much responsibility†:

vector<unique_ptr<Projectile>> projectiles;
// ...
projectiles.emplace_back(new Projectile(&m, x, 2, y));

Please make sure that you know the lifetime of m, too. In general, I recommend taking a good long look at all your objects and figuring out:

  • which piece of code is responsible for their lifetime, and
  • what that lifetime is

before you start throwing pointers at everything. :)

† In particular, avoiding the need to delete.

Upvotes: 3

Related Questions