Memory leak safe?

[Bogeyman]

I imagine you might see more dramatic results if you tried to modify the contents of the memory pointed to by map2.grid, after deleting map1.

I tried what you suggested here:

#include <iostream>
#include "Player.h"
#include "Map.h"

using namespace std;

void func(Map);

int main()
{
    Map map1(1, 26);
    Map map2 = map1;

    func(map1);

    map1.draw();
    map1.display();

    map2.draw();
    map2.display();

    return 0;
}

void func(Map object)
{

}

This program does crash and I think it's because the destructor for map1 is called at the end of the scope of func(), de-allocating the pointer memory from the objects map1 and map2, then the destructors for both objects are called again at the end of the scope of main(), attempting to de-allocate already de-allocated memory.

What I don't understand is, why does this cause a crash, but not the two destructors being called at the same time at the end of main()? Is it that the act of de-allocating the same memory more than once is not sufficient to cause a crash, but any attempts to access the de-allocated memory afterwards is?

[cire]

What I don't understand is, why does this cause a crash, but not ...

You don't seem to be taking up what's being put down. This is the realm of undefined behavior.

The behavior of the code is undefined. It may crash or it may appear to be entirely innocuous.

[Bogeyman]

Okay then, I guess it's all just undefined behaviour, regardless of when the destructor is called. I was only puzzled at the fact that when calling the destructor for the two objects only once in the program, it finishes execution normally, but I see now that that's probably because nothing tries to access the de-allocated pointer memory after the destructor is called in that situation.

Anyway, I had a go at setting up a copy constructor and handling negative values in the call to the overloaded constructor. Would you have a look and let me know if they're okay?
Overloaded Constructor:

Map::Map(int x, int y) : row(x), col(y)
{
    // Set row and col to 1 if user entered value less than 1.
    if(row < 1){ row = 1; cout << "Row has been set to 1.\n"; }
    if(col < 1){ col = 1; cout << "Col has been set to 1.\n"; }

    // Point grid to and allocate a new area of memory
    // equal to (row * col * sizeof(data type)) bytes
    grid = new char[row * col];
}

Copy Constructor:

Map::Map(const Map &obj) : row(obj.row), col(obj.col),
grid(new char[sizeof(obj.grid)])
{

}

There's no handling for when there's not enough memory left to be allocated, but I'll have to learn that later. As MikeyBoy said, exception handling is an advanced topic. For now I'd just like to know if the implementation itself is okay.

[MikeyBoy]

It looks OK to me, but I don't see the point of assigning row and column numbers of 1, and allocating a 1 x 1 grid, when x and y are less than 1. Surely a better default state would be to set them to zero, and not allocate a grid?

[Bogeyman]

Yeah, that sounds good. The draw() and display() functions would be fine as well, since the loops would never execute under those conditions. I suppose then I could provide some setter methods to allow the user to set the members later, so they won't be stuck with a non-existent map.

Thanks MikeyBoy, you've been a great help, I really appreciate it.

[Disch]

A few things:

1) No need to check for a null pointer before deleting. Deleting a null pointer has no effect.

Map::~Map()
{
    delete[] grid;  // <- this is perfectly safe. No need to check for null
}

2) You probably shouldn't be using new[]/delete[] anyway. It makes the code more difficult to write and more error-prone. This is very easily accomplished with a container class like a vector.

3) 'i' and 'j' are temporary variables for loop counters, and are not part of the object's state. They should be local to whatever function they are used in, and should not be members of the class.

4) Your logic for printing is wrong. You can't do 'grid + x + y'... as this means that 0,2 and 2,0 would be the same tile. Typical form is 'grid + (y*width) + x'

5) 'draw' is poorly named. That function does not draw anything, it merely clears/fills the contents of the map. 'fill' would be a more appropriate name.

6) Personal preference, but I tend to keep all file names lowercase as this leads to less confusion on case-sensitive file-systems.


Your code with recommended changes:


map.h

#ifndef MAP_H_INCLUDED
#define MAP_H_INCLUDED

#include <vector>

class Map
{
    public:
        Map()  {}               // <- no body necessary
        Map(int, int);
                                // <- no destructor necessary, vector automatically cleans up.
                                // <- no copy ctor/assignment necessary, vector automatically does it
                                // <- no move ctor/assignment necessary, vector automatically does it

        void fill(char fillval = '0');
        void display();

    private:
        int                 width = 0;
        int                 height = 0;
        std::vector<char>   grid;
};

#endif // MAP_H_INCLUDED  

map.cpp

#include <iostream>
#include "map.h"

using namespace std;

Map::Map(int x, int y)
    : width(x)
    , height(y)
    , grid(width*height, '0')   // <- size of width*height, default value of '0' for all elements
{
}

void Map::fill(char fillval)
{
    for(auto& i : grid)
        i = fillval;
}

void Map::display()
{
    for(int y = 0; y < height; ++y)
    {
        for(int x = 0; x < width; ++x)
        {
            cout << grid[ y*width + x ];
        }
        
        cout << '\n';
    }
}

[Bogeyman]

Thanks for the advice Disch. I have a few questions though.

If you were to use raw pointers, do you think it would be good measure to set the pointers to NULL after de-allocating memory from them, or is that overkill in terms of ensuring safety from undefined behaviour?

In your fill() function declaration, does creating the parameter like that basically mean that you can provide your own value to set the elements of the vector to, but you can also call fill() without passing a value and it will set the elements to '0'? Kind of like a default and overloaded function in one.

void Map::fill(char fillval)
{
    for(auto& i : grid)
        i = fillval;
}

Is this a new kind of for loop introduced in C++11?

grid(width*height, '0')
Does this set the number of elements in grid to width * height? Is the additional parameter '0' in this initialisation something unique to vectors? I'm not familiar with vectors and I've never seen this kind of initialisation before.

[Disch]

If you were to use raw pointers

I wouldn't ;P. If you are manually cleaning up, you are probably doing something wrong.

But anyway....

do you think it would be good measure to set the pointers to NULL after de-allocating memory from them, or is that overkill in terms of ensuring safety from undefined behaviour?

It isn't strictly necessary (especially if done in a destructor)... but it doesn't hurt to do it.

It's really a case of "better safe than sorry" -- so yeah it is generally a good idea to null your pointers after deleting them. That's a good habit to have.

(though a better habit is to use smart pointers and container classes so you don't have to deal with this at all)

In your fill() function declaration, does creating the parameter like that basically mean that you can provide your own value to set the elements of the vector to, but you can also call fill() without passing a value and it will set the elements to '0'?

Yes. This is an "optional parameter". When calling the function, the parameter is optional, and if omitted, '0' would be used.

obj.fill();    // <- fills with '0'
obj.fill('0'); // <- same
obj.fill('#'); // <- fills with '#' 

Is this a new kind of for loop introduced in C++11?

Yes. It's a "range based for loop" and it makes traversing arrays and container classes much easier.

It's sort of like the C++ version of the "foreach" construct seen in other languages. In my code example, 'i' would be the next element in the vector each iteration of the loop.

Does this set the number of elements in grid to width * height?

Yes

Is the additional parameter '0' in this initialisation something unique to vectors?

No, other container classes have it as well.

string, deque, and list all have it as well. Any container where you can explicitly set the size of it will allow you to provide an initialization value.

[Bogeyman]

Thanks Disch!