Would this cause a memory leak?

Okay, so I am having somewhat of a disagreement with someone else, and I was hoping someone who knows more about c++ than either of us can clear this up. Say we have this block of code somewhere inside a function(for a tilemap engine):

void loadTiles()
{
Tile* tile = new Tile();
Level->addTile(x, y, tile); //x and y are just arbitrary ints.
/* when addTile is called, it fills the values of the chunk of memory pointed to by tile to the predefined chunk of memory created in the Level object. */
//Then, to remove the dangling pointer safe开发者_StackOverflow社区ly,
tile = NULL;
} //Then the actual memory pointed to by tile is deallocated here.

The Level class has a 2D array of Tiles called map[][], and it's addTile function looks exactly like this:

void Level::addTile(int x, int y, Tile *tile) 
{
    map[x][y] = tile;
}

The memory pointed to tile was deallocated, the pointer is no longer pointing to a nonexistant object, and the values for the tile object were essentially copied into the Level object's map[][] array. Am I right, or am I mistaken? The other person argues that this will lead to a memory leak.

---

Let's look at each piece of code.

1) Allocating memory

Tile* tile = new Tile();

This creates a new Tile object on the heap and stores the memory address in the variable tile. Keep in mind that the variable tile is only a pointer, not the object itself.

2) Copying the reference

void Level::addTile(int x, int y, Tile *tile) { map[x][y] = tile;}

The above function simply takes a pointer and saves it into a multi-dimensional array for future use. In the context of all of the code listed, there will now be TWO references to the object...the tile* in the original calling function and the entry in the multi-dimensional array. Again, keep in mind that these are pointers (just 4 bytes, depending on your system architecture).

3) Setting a pointer to NULL

tile = NULL;

The result of this code is that the pointer variable tile will no longer point to the created object on the heap. The object will still exist, though. At this point, in the context of all of the code, you will still have a pointer to the object due to the map[][] array.

In reality, you do not need this line of code. tile isn't a dangling pointer, as it points a valid object (before you set it to NULL). The code does not destroy the object either. Since tile is a local variable, it will be cleaned up when it function scope exits. Only the pointer will be cleaned up, not the object it points to. Setting it to NULL in this scenario accomplishes very little except waste cycles.

It is also not a memory leak per say. You still have a valid pointer to the object in the map[][] array, so you can always use that reference to clean up the memory.

4) What you need to do

You need to delete the object somewhere. In C++, this is the delete keyword.

delete tile;

OR

delete map[x][y];

Now, keep in mind, once the above code runs, the memory on the heap will be freed back to the operating system and your application can no longer access the memory safely. Therefore, any call to map[x][y]->{SomeMethod} would result in a access violation exception. The pointer stored in map[x][y] is now a dangling pointer (it is pointing to a memory address that not valid for that type).

If you need to remove the tile from the level map before you destroy the level, you would do this:

void Level::deleteTile(int x, int y) 
{ 
    if (map[x][y] != NULL)
    {
        delete map[x][y];
        map[x][y] = NULL;
     }
}

I would also change the addTile method to something like this:

void Level::addTile(int x, int y, Tile *tile)
{
    deleteTile(x, y);
    map[x][y] = tile;
}

Lastly, when you are destorying a Level object, you'll need to do something like this:

void ~Level()
{
    for (int i; i<MaxX; i++)
    {
        for (int j; j<MaxY; j++)
        {
            delete map[i][j];
        }
    }
}

When you're creating a Level object, you should zero out the map[][] array so that all values will be NULL as well. Since the map array is not local to a function, it is a good idea to set all of its pointer values to NULL so that you know when it contains a valid pointer and when it doesn't.

If you dynamically create (using the new keyword) the map[][] array, you'll need to clean up its memory using the delete keyword too.

Hope this helps.

---

This will leak memory. There is absolutely no code in the posted scope that would destroy the Tile object you allocated dynamically. You aren't copying any values anywhere- they're all pointers, which are references. There's no evident reason that you did dynamic allocation, either, and if as you say, map is a Tile[][], then I'm amazed that this even compiles.

---

A textbook example of a leak if you do not delete it from the map later on. And no, the memory is of course not deallocated. Why would it? Also, you're not moving the pointer or anything, after calling addTile, the tile pointer is still valid.

---

No, this basically transfers ownership from tile to map[x][y] in the object Level. You should ensure that the pointed object is freed later (i.e., in the destructor of Level. You should also change your addTile() method to check that map[x][y] isn't pointing to anything, otherwise the memory previously pointed by it will leak.

---

The memory pointed to tile was deallocated, the pointer is no longer pointing to a nonexistant object, and the values for the tile object were essentially copied into the Level object's map[][] array.

Sorry, but you are mistaken on each of your points.

• "The memory pointed to [by] tile was deallocated.

The general rule is this: every new must have exactly one matching delete. Since you allocated the memory using new and never invoked delete, the memory is never deallocated.

• "The pointer is no longer pointing to a nonexistent object"

The variable called tile in the anonymous code snippet points to the allocated memory until NULL is assigned to it. The variable called tile in Level::addTile points to the allocated memory for its entire lifetime.

• "the values for the tile object were essentially copied into the Level object's map[][] array"

The value of the pointer, not the value(s) of the object were copied.

The following code snippet does what you think your code did:

{
  Tile tile;
  Level->addTile(x, y, tile); //x and y are just arbitrary ints.
  /* when addTile is called, it fills the values of the chunk of memory pointed to by tile to the predefined chunk of memory created in the Level object. */
} //Then the actual memory pointed to by tile is deallocated here.

void Level::addTile(int x, int y, Tile &tile) 
{
    map[x][y] = tile;
}

---

I am in no way an expert on memory management in C++, but I would say that what you are doing will not result in memory leaks (given that you do properly delete the memory later on somewhere). My argumentation for this is that you still have a valid pointer to the memory you allocated, hence you can still delete is by using the 'delete' keyword.

Think of pointers as memory-addresses. When you allocate memory by using 'new', you should make sure that you store that address somewhere so that you can actually access the data. The address is stored in/as a pointer. So if you lose your pointer (by going out of scope, as in your example with the block of code), you lose the ability to free the allocated memory because you can no longer know where the data is in memory. In your example, however, you still keep a pointer in your 'map'-array, so you should still be able to release the allocated memory.

The key thing to remember here is that a pointer is NOT the data itself. Losing a pointer is not bad as long as you still have another pointer to the data. Once you lose ALL your pointers to data, then you're heading for the realm of memory leaking.

---

This allocates the object on the heap :

Tile* tile = new Tile();

This does nothing :

tile = NULL;
} //Then the actual memory pointed to by tile is deallocated here.

Since the object is not deleted, it is a memory leak.

---

Make the map a 2D array of pointers to Tile.

Then, when you are ready to release the resources, delete all the elements in map and then delete the map itself (if allocated with new).

This way you won't leak memory.

---

There's no deallocation going on in the code you posted. You allocate a new Tile object and the addTile() implementation is taking ownership of it.

There's no memory leak per se; it depends on the implementation of the Level class, which is holding the allocated Tile object. If at some point it deletes the tile objects from the internal map, then there is no memory leak, otherwise, there is.