Pushing local objects into a list

Question

I have a class

class Invader
{
public:
    Invader();
    ~Invader();
public:
    void Init(InvaderTypes invadertype, CIw2DImage *AlienImage);
    void Update(float dt);
    void Render();
    void SetAlienImage(CIw2DImage *image){ ImageAlien = image; }

    void        setVisible(bool show)       { Visible = show; }
    bool        isVisible() const           { return Visible; }


    Iw2DSceneGraph::CSprite         *AlienSprite;
    Iw2DSceneGraph::CAtlas          *AlienAtals;
    CIw2DImage                      *ImageAlien;
    std::list<Bullet*>              *Bullets;
    CIwFMat2D                       Transform;              // Transform matrix

    bool                             Visible;                // Sprites visible state
    bool                             Canfire;
};


void Invader::Init(InvaderTypes invadertype, CIw2DImage *AlienImage)
{
    if (invadertype == InvaderTypes::TOP_ALIEN)
    {
        //SetAlienImage(AlienImage);
        mImageAlien = AlienImage;
        // Create EnemyTop atlas
        int frame_w = (int)(mImageAlien->GetWidth() / 2);
        int frame_h = (int)(mImageAlien->GetHeight());
        AlienAtals = new CAtlas(frame_w, frame_h, 2, mImageAlien);
        AlienSprite = new CSprite();
        AlienSprite->m_X = 0;
        AlienSprite->m_Y = 0;
        AlienSprite->SetAtlas(AlienAtals);
        AlienSprite->m_W = (float)AlienAtals->GetFrameWidth();
        AlienSprite->m_H = (float)AlienAtals->GetFrameHeight();
        AlienSprite->m_AnchorX = 0.5;
        AlienSprite->SetAnimDuration(2);
    }
    else if (invadertype == InvaderTypes::MIDDLE_ALIEN)
    {

    }
    else if (invadertype == InvaderTypes::LAST_ALIEN)
    {

    }


    Visible = true;
    Bullets = new std::list<Bullet*>();
    Canfire = true;
}



Invader::Invader()
    {

    }
    Invader::Invader(const Invader&other)
    {
        AlienAtals = new CAtlas();
        AlienSprite = new CSprite();
        *AlienAtals = *other.AlienAtals;
        *AlienSprite = *other.AlienSprite;
    }

I try to initialize it by:

list<Invader> *invaders = new list<Invader>();

    int spacing = 10;
for (int i = 0; i < 5; i++)
{
    Invader invader;
    invader.Init(TOP_ALIEN, gameResources->getAlienImageTop());
    invader.AlienSprite->m_X = 50 + spacing;
    invaders->push_back(invader);
    spacing += 50;
}

After pushing the object invader to the list, at the end the invaders list holds pointers that are not initialized. All the pointers got lost the references. I wonder why ?

`list *invaders = new list();` Why do you need to create a `std::list` dynamically? It's as if you're writing Java, not C++. Same here: `Bullets = new std::list();` Just create a `std::list` -- I see no reason for pointers or `new` to create the list. — PaulMcKenzie, Dec 31 '14 at 17:03
@PaulMcKenzie yea sometimes mixing java and C# of day job, make one confuse so much — andre, Dec 31 '14 at 17:06
Don't mix Java or C# in C++. You're creating potential for memory leaks for no reason. For example, all you need is `std::list Bullets;` as the member variable, and this line: `Bullets = new std::list();` can be removed. — PaulMcKenzie, Dec 31 '14 at 17:07

score 3 · Accepted Answer · answered Dec 31 '14 at 17:07

3

The problem, I assume, is what happens in ~Invader(). Let's simplify the example a ton:

struct A {
    int* p;
    A() { p = new int(42); }
    ~A() { delete p; }
};

A just manages a pointer. When an A goes out of scope, that pointer gets deleted. Now, what happens when we do this:

list<A> objs;
{
    A newA;
    objs.push_back(newA);
    // newA deleted here
}
// objs has one element... but its pointer has been deleted!

The problem is that copying A (which push_back() does) just performs a shallow copy: we copy our pointer. But since A manages its own memory, we need to do a deep copy. That is:

A(const A& rhs)
: p(new int(*(rhs.p)))
{ }

That way, the copied A won't double delete the same pointer. With C++11, this can be much more easily managed with just:

struct A {
    std::shared_ptr<int> p;
    A() { p = std::make_shared<int>(42); }
    ~A() = default; // this line not even necessary
};

Here, copying A will copy the shared_ptr and both copies of A will have a valid object to point to. If you can't use C++11, you can still use boost::shared_ptr<T> for all your memory management needs. And if you can't use that, then you have to write a copy constructor that does a full copy of all your pointer elements.

Or, the simplest solution, would be to just make your container have pointers:

list<A*> objs;
objs.push_back(new A);

Then the "shallow copy" is the right thing to do, and all you need to do is remember to delete everything in the container at the end.

answered Dec 31 '14 at 17:07

Barry

286,269
29
621
977

I modified the code, and still all the pointers don't refer to anything list *invaders = new list(); int spacing = 10; for (int i = 0; i < 5; i++) { Invader *invader = new Invader(); invader->Init(TOP_ALIEN, gameResources->getAlienImageTop()); invader->AlienSprite->m_X = 50 + spacing; invaders->push_back(invader); spacing += 50; } – andre Dec 31 '14 at 17:15
@andreahmed I don't know what "pointers don't refer to anything" means. That looks valid as far as putting stuff in the list goes though. Perhaps ask a new question? – Barry Dec 31 '14 at 17:19
Sorry. I mean when I tried the simple solution to let the container have pointers, I lost the reference to pointers that are inside the object Invader. That was the same behavior without using that simple solution – andre Dec 31 '14 at 17:21
@andreahmed What does "I lost the reference to pointers" mean? – Barry Dec 31 '14 at 17:25
I mean the pointers in the class, have nothing to refer to. they are not initialized. However it was initialized in the init function, but it was lost in the list after pushing it in to the list. – andre Dec 31 '14 at 17:27
@andreahmed You sure they were initialized? They wouldn't magically become not initialized during `push_back`, especially if that's just doing one pointer copy. – Barry Dec 31 '14 at 17:34
I will post the new updated code. Please take a look – andre Dec 31 '14 at 17:43

score 0 · Answer 2 · edited Dec 31 '14 at 17:10

Your list contains Invader objects. When you store them in the list, the Invader copy-constructor is called and a copy of the variable invader is stored in the list.

Unless you define a copy-constructor, it will be default simply do a shallow-copy of your object. This may not be what you want. You should write an explicit copy-constructor to make sure that the Invader is copied correctly.

Another solution would be to dynamically allocate Invader objects using the new keyword and storing pointers in the list. If you do that, be careful to make sure that you call delete on the Invader objects in the list when you are done with them.

@andreahmed I don't understand your question. – Daniel Dec 31 '14 at 17:05 — Daniel, Dec 31 '14 at 17:05

Pushing local objects into a list

2 Answers2

Linked