[SDL] I'm making a snake game and apparently I have a memory leak.

[Malaxiz]

The snake is made up of blocks. The snake class has a static vector of pointers to the snake class and each snake class (I only use one snake object called player) has a vector of pointers to the block class. Apperently I have a memory leak because I'm using the new thing alot but I'm not deleting anything. The problem is I don't know how to properly delete.

This is the main code for the snake and block classes:

CSnake.h

#ifndef CSNAKE_H
#define CSNAKE_H

#include <vector>
#include "CBlock.h"

class CSnake
{
public:
    CSnake();

    static std::vector<CSnake*> SnakeList;

    std::vector<CBlock*> BlockList;

    CBlock* Head;

public:
    void OnLoop();

    void OnRender(SDL_Surface* Surf_Destination);

public:
    void AddBlock();

    void SetHeadPos(int x, int y);

    CBlock* PrevParent;

    int VelX;
    int VelY;

};

#endif // CSNAKE_H 

CSnake.cpp

#include "CSnake.h"

std::vector<CSnake*> CSnake::SnakeList;

CSnake::CSnake()
{
    VelX = 0;
    VelY = 0;

    CBlock* Head = new CBlock(100, 100);

    PrevParent = Head;

    BlockList.push_back(Head);
}

void CSnake::OnLoop()
{
    for(int i = 0; i < BlockList.size(); i++)
    {
        if(!BlockList[i]) continue;

        BlockList[i]->OnLoop(VelX, VelY);
    }
}

void CSnake::OnRender(SDL_Surface* Surf_Destination)
{
    for(int i = 0; i < BlockList.size(); i++)
    {
        if(!BlockList[i]) continue;

        BlockList[i]->OnRender(Surf_Destination);
    }
}

void CSnake::AddBlock()
{
    CBlock* Block = new CBlock(-20, -20);

    Block->Parent = PrevParent;

    PrevParent = Block;

    BlockList.push_back(Block);
}

CBlock.h

#ifndef CBLOCK_H
#define CBLOCK_H

#include <SDL.h>
#include <vector>
#include "CRect.h"
#include <iostream>

class CBlock
{
public:
    CBlock(int x, int y);

public:
    int PosX;
    int PosY;
    int NewPosX;
    int NewPosY;

    CBlock* Parent;

public:
    void OnRender(SDL_Surface* Surf_Destination);

    void OnLoop(int VelX, int VelY);

};

#endif // CBLOCK_H 

CBlock.cpp

#include "CBlock.h"

CBlock::CBlock(int x, int y)
{
    PosX = x;
    PosY = y;

    Parent = NULL;

    NewPosX = x;
    NewPosY = y;
}

void CBlock::OnRender(SDL_Surface* Surf_Destination)
{
    CRect::CRect_RenderRect(PosX, PosY, 20, 20, Surf_Destination, 0xFF, 0xFF, 0xFF);
}

void CBlock::OnLoop(int VelX, int VelY)
{

    PosX = NewPosX;
    PosY = NewPosY;

    if(Parent != NULL)
    {
        NewPosX = Parent->PosX;
        NewPosY = Parent->PosY;
    }
    else // This means this is the head obj.
    {
        NewPosX = PosX + VelX * 20;
        NewPosY = PosY + VelY * 20;
    }
}

This is my current Cleanup function in CApp:

#include "CApp.h"

void CApp::OnCleanup()
{
    for(int i = 0; i < CSnake::SnakeList.size(); i++)
    {
        if(!CSnake::SnakeList[i]) continue;

        CSnake::SnakeList[i]->BlockList.clear();
    }

    CSnake::SnakeList.clear();

    SDL_Quit();
}

How should I properly memory management?

[xismn]

Okay, so you're using new in two functions:

1.) The CSnake constructor
2.) The CSnake::AddBlock() function

You're already pushing the Block pointers onto a vector. That's good, because then I don't have to tell you what a vector is.

As you've probably realized, your clean up function is not sufficient.
The easier and more modern solution would be to use smart pointers.
Smart pointers are supported by:
1.) C++11 std
2.) C++ Technical Report 1
3.) Boost Library

I've elected to use TR1, but you can use whichever. Here is some pseudo code to demonstrate the simplicity.

#include <vector>
#include <memory>

class Block {
public :
	//whatever
	int foo;
};

typedef std::tr1::shared_ptr<Block> BlockPtr;

class Snake {
public :
	Snake() {}
	void addBlock();

	std::vector<BlockPtr> blocks;

};

void Snake::addBlock() {
	blocks.push_back(BlockPtr(new Block()));
}

int main() {
	Snake snake;
	snake.addBlock();
	return 0;
}

Notice, the lack of clean up routines and destructors.
shared_ptr<> is one of the smart pointers available. The keyword here, is automated memory management. To understand how it works, either look here http://en.wikipedia.org/wiki/Smart_pointer, or a google search should yield good results.

The other alternative would be to loop through your vector and delete in your destructor, but you would need some way of making sure that what you're deleting has been allocated in the first place.
For instance, if for whatever reason, new fails, you should have some way of catching bad_alloc. You can see why I advocate smart pointers.

[Malaxiz]

Hi thank you for the answer. I've tried doing the loop you said and this is how it looks:

#include "CApp.h"

void CApp::OnCleanup()
{
    std::vector<CBlock*>::iterator it;

    for ( it = Player->BlockList.begin(); it != Player->BlockList.end(); )
    {
      delete * it;
      it = Player->BlockList.erase(it);
    }



    for(int i = 0; i < CSnake::SnakeList.size(); i++)
    {
        if(!CSnake::SnakeList[i]) continue;

        //CSnake::SnakeList[i]->BlockList.erase(); // Not working... needs some kind of vector iterator argument

        CSnake::SnakeList[i]->BlockList.clear();
    }

    CSnake::SnakeList.clear();

    SDL_Quit();
}

Will this work? I'll look into the smart pointers but will this be good enough meanwhile?

[xismn]

Lines 5 - 11 look okay. Usually, the things that make the mess should clean it up as well (the CSnake object), but you can do whatever you want.
Lines 15 - 24 are questionable, because all you're doing is clearing the block vector of each Snake if the CSnake* doesn't point to NULL - but you're not deleting the memory. Like I said above, if you let each Snake handle it's own clean up during deconstruction, you don't need verbose clean up methods. Another major problem with your set up, is that snake blocks only get deleted when the application terminates. What if I'd like to remove a snake while I'm playing the game? The application hasn't terminated, and so the memory won't get deleted.

Something like this might be preferable:

CSnake::~CSnake() {
	for(std::vector<CBlock*>::iterator it=BlockList.begin(); it!=BlockList.end(); ) {
		delete (*it);
	}
	BlockList.clear();
}

void CApp::OnCleanup() {
	CSnake::SnakeList.clear();//calls the CSnake deconstructor for each snake, which deletes all the blocks of each snake respectively.
	//do whatever else you need to do
	SDL_Quit();
}

But just for the sake of completeness - since you're not using smart pointers, just to reiterate, it's a good idea to set up exceptions to catch a potential bad_alloc, so that you're not deleting memory which wasn't allocated in the first place.

[Malaxiz]

This is how I made the CSnake deconstructor:

CSnake::~CSnake()
{
    for(std::vector<CBlock*>::iterator it = BlockList.begin(); it != BlockList.end(); it++)
    {
        try
        {
            delete (*it);
        }
        catch(std::bad_alloc){
        }
    }
    BlockList.clear();
}

Is that how you mean by catching a bad_alloc?

[xismn]

Not quite. You should try to catch a bad_alloc when you allocate memory, not when you delete it.
Perhaps I stressed the importance of catching this kind of error too much, because I remember never catching exceptions when I first got into this field of programming, and never having any problems. The point is that it's good practice.
The idea is that you attempt to add objects using new. If new fails, you don't allocate memory, and you don't push_back a new block to the snake. This would result in the snake eating fruit, but then not growing - and staying the same size. Later on when you are deleting memory, you don't have to do any exception handling, because the elements in the block vector are guaranteed to point to allocated memory.

void CSnake::AddBlock() {
	try {
		Block* temp = new Block();
		BlockList.push_back(temp);
	}
	catch(std::bad_alloc) {/*"good enough"*/}
}

[Malaxiz]

Thanks! This is the code now for AddBlock:

void CSnake::AddBlock()
{
    try
    {
        CBlock* Block = new CBlock(-20, -20);

        Block->Parent = PrevParent;

        PrevParent = Block;

        BlockList.push_back(Block);
    }
    catch(std::bad_alloc){/*"good enough"*/}
}

And I've been thinking, wouldn't we need to delete the CSnake (it's called player) object too in OnCleanup? Like the CBlocks? Or does the deconstructor do that already in CSnake?

[xismn]

Oops you're right, I didn't see that. I'm making an assumption, that you are using new to add new CSnake pointers to the SnakeList vector. It's an assumption, because you haven't actually provided code to confirm that, but whatever.

Yes, you will also need to take care of deleting CSnake pointers in the SnakeList vector (Only if they have been new'd). The CApp::OnCleanup() function seems like a good candidate to do this.

void CApp::OnCleanup() {
	for(std::vector<CSnake*>::iterator it=CSnake::SnakeList.begin(); it!=CSnake::SnakeList.end(); ) {
		delete (*it);
	}
	CSnake::SnakeList.clear();
	//whatever else you need to do

	SDL_Quit();
}

[cire]

I'm not sure why you're using new at all here. Blocks don't need to form a list. The vector they're contained in provides the structure you need.

This looks to be way more complicated than it needs to be.

[j3Rz8vqX]

cire is correct.

You're essentially handling one step that the vector can already do for you.

Your idea is moving towards the right direction, but wouldn't it be easier to let the vector handle the declaration of your new objects?

Since you're already using vector, you should consider letting it do it's job.

You could possibly use the vector to push_back block, then pass its address to whoever wants/needs it's address.

this would eliminate the need of new in your program.

std::vector<CSnake> CSnake::SnakeList;
CSnake::CSnake()
{
    VelX = 0;
    VelY = 0;

    //CBlock* Head = new CBlock(100, 100);
    //PrevParent = Head;
    //BlockList.push_back(Head);

   CBlock Head(100,100);
   PrevParent = &Head; //guessing PrevParent wants an address;
   BlockList.push_back(Head);
}

Same result, different means; without "new".
Let the vector work for you(let it handle construction/destruction), it's an Object with array capabilities and more, not merely an array of Objects; hence why some folks disfavors it (extra features may or may not be favorable... aka bloat).

Even if you have pointers in your class object, because you're passing it by address(not a copy), it will retain its data, identity, and signature.

The only time I was "forced" to use new, was in a situation where an object would immediately destroy itself if it deemed itself inactive (a lib that I had to workaround - No names mentioned, useful in its own, but not flexible at all). Hopefully this isn't your case.

Good Luck.

[Malaxiz]

Thanks! I did something like this at first, but it didn't work. After the function was done the Head object got deleted automatically. This is how the code looked:

#include "CApp.h"

bool CApp::OnInit()
{
    if(SDL_Init(SDL_INIT_EVERYTHING) < 0) return false;

    if((Surf_Display = SDL_SetVideoMode(640, 480, 32, SDL_HWSURFACE | SDL_DOUBLEBUF)) == NULL) return false;

    CBlock Head(400, 200);

    CBlock Body(200, 100);

    CBlock::BlockList.push_back(&Head);
    CBlock::BlockList.push_back(&Body);

    return true;
}

(why aren't you using the & sign before Head when you are pushing it in the BlockList?)

[cire]

Head and Body cease existing when CApp::OnInit returns, so the pointers you put in BlockList are invalid outside the member function.

I would suggest not using pointers, and just storing the blocks directly in the vector. (There is also no reason for a block to contain a pointer to another block.) By convention, the first block in the vector will be the head and the following blocks will be the blocks in the body.

[j3Rz8vqX]

The code below is what you originally had:

    CBlock Head(400, 200); //created an object in this functions scope
    CBlock Body(200, 100); //created an object in this functions scope

    CBlock::BlockList.push_back(&Head); //copied the objects address
    CBlock::BlockList.push_back(&Body); //copied the objects address 

This wouldn't work, because as soon as you left the scope of "CApp.OnInit()", both CBlock Head and Body would be discarded, local variables in class_methods and functions cease to exist after leaving scope.
Only leaving you with reference to non-allocated memory blocks.

In mines:

   CBlock Head(100,100); //created an object in this functions scope
   //PrevParent = &Head; //Edit: My mistake, Head no longer persists
   BlockList.push_back(Head); //copy the object to the end of vector
   revParent = &BlockList[BlockList.size]; //give the address of vectors last element 

Cbock Head would still be local, therefore not persisting after leaving the method - dead blocks.

BlockList copies Head and pushes it to the last element - BlockList vector persists outside of this method's scope, giving life and taking ownership of new objects with copied parameters of Head.

My original revParent wouldn't work as well, you'd want to give it the address of the vector's copy, not the address of this local Head.
//Edited
revParent receives the an address that will still persist after leaving the scope of the OnInit method.

As for not using the &(address/reference) sign before Head, I've simply decided that I was going to let my vector contain and handle real Objects, and not just pointers to those objects.

Hence:

std::vector<CSnake> CSnake::SnakeList;

It would be similar to asking why you'd want to declare primitives rather than address.

int i;
char c;
float f;

Rather than say..

int * i;
char * c;
float * f;

You can change every primitive in your code to pointer type and assign them new data, but it isn't necessary. By doing that, your basically adding a buffer between you and access of the data.

Same applies to your vector of object pointers.

Why work with pointers when you can access the data directly?

Should the vector not hold data?

The vector is already pointing to many objects/variables. Why point to a point that points to an object/variable?

----

Side note:
AMD mantle is supposedly giving applications/users direct access to GPU without access the CPU.

Original: (somewhat like pointer - no direct access)
APP->CPU->GPU

Mantle: (address to variables - direct access)
APP->GPU

No flame wars please
----
Unless you must use vector pointers, I would suggest on not using pointers (for the vectors objects that is). Having pointers in Objects that point to persistent data in vectors are okay, as long as someone actually has real and maintained data. Preferably the vector should have the higher priority when compare to object, which is what the vector is doing for you.

You could possibly make do with pointers, by doing... [Edited - Removed]
Nah...

Listen to cire:

This looks to be way more complicated than it needs to be.