Memory leak safe?

[Bogeyman]

Would anyone be willing to have a look at my code and tell whether or not it is safe from memory leaks?

Also, is it safe to set grid to NULL when the default constructor is called, considering grid is involved in the draw() and display() functions regardless of whether or not you provide parameters in the call to the constructor in main()?

main.cpp

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

using namespace std;

int main()
{
    Map map1(5, 10);

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

    return 0;
}

Map.h

#ifndef MAP_H_INCLUDED
#define MAP_H_INCLUDED

class Map
{
    public:
        Map();
        Map(int, int);
        ~Map();

        void draw();
        void display();

    private:
        int row, col, i, j;
        char *grid;
};

#endif // MAP_H_INCLUDED 

Map.cpp

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

using namespace std;

Map::Map() : row(0), col(0), i(0), j(0), grid(0)
{

}

Map::Map(int x, int y) : row(x), col(y), i(0), j(0),
grid(new(nothrow) char[row * col])
{
    if(grid == NULL)
    {
        cout << "Memory allocation failed.\n";
    }
}

Map::~Map()
{
    delete[] grid;
    grid = 0;
}

void Map::draw()
{
    for(i = 0; i < row; ++i)
    {
        for(j = 0; j < col; ++j)
        {
            *(grid + i + j) = '0';
        }
    }
}

void Map::display()
{
    for(i = 0; i < row; ++i)
    {
        for(j = 0; j < col; ++j)
        {
            cout << *(grid + i + j);
        }

        cout << endl;
    }
}

[koothkeeper]

At a cursory look, I think it's safe: However, you should check grid in your destructor before deleting it: Especially since new in the second ctor of Map may have failed.

[Bogeyman]

You mean something like this?

Map::~Map()
{
    if(grid != NULL)
    {
        delete[] grid;
        grid = 0;
    }
}

[koothkeeper]

Exactly.

[Bogeyman]

When using the default constructor, grid is set to NULL a second time. Is that safe? Also, since grid has not been allocated any memory, what happens when I try to de-allocate memory with delete[]?

EDIT:
Actually, I think it's okay. Since grid is set to NULL in the default constructor, the code inside the destructor never tries to clean it up.

[ne555]

> you should check grid in your destructor before deleting it
no need, deleting NULL does nothing, it's perfectly safe.


> When using the default constructor, grid is set to NULL a second time.

Map::Map() : row(0), col(0), i(0), j(0), grid(0)
{

}

that's the only time that you set `grid', ¿where is the second time that you are talking about?


> *(grid + i + j)
¿what do you think you are doing?

[Bogeyman]

that's the only time that you set `grid', ¿where is the second time that you are talking about?

Check my edit. I realise the destructor doesn't try to set grid to NULL a second time because of the if condition.

> *(grid + i + j)
¿what do you think you are doing?

Accessing the elements of the dynamically allocated array.

[ne555]

yes, ¿but do you realise which elements?
suppose that row = 2 and col = 3
you'll access

0+0=0, 0+1=1, 0+2=2
1+0=1, 1+1=2, 1+2=3

so, from the 6 elements that you allocated you only access 4 (and notice the repeated access)


It doesn't matter how many times you set a pointer to NULL.
However, ¿what's the point of doing it in the destructor of the object? you may never be able to access it after that.

[Bogeyman]

so, from the 6 elements that you allocated you only access 4.

If that's the case then why does my code run as I intend. The array is fully filled with 0's. Compile and run the code and see for yourself.

what's the point of doing it in the destructor of the object?

Just a safety precaution suggested by a tutorial. It makes sure that if you try to use the pointer thereafter, you won't do any damage to the memory it was allocated.

[ne555]

> If that's the case then why does my code run as I intend
because you suck at testing.
put a different value in each cell.

[Bogeyman]

There's no need to be insulting. I simply did not see how what I was doing was incorrect based on what I was trying to achieve.

[coder777]

It is not only safe to set grid to NULL (or something other valid like new) in your constructor, it is basically required in order to avoid undefined behaviour.

*(grid + i + j) calculates the index incorrectly.
The correct calculation is: *(grid + (i * (col - 1)) + j).

Note that this notation grid[(i * (col - 1)) + j)] is also valid.

Note that you may have a problem with shallow copy:

int main()
{
    Map map1(5, 10);
    Map map2 = map1; // This will crash because both map1 and map2 try to delete the same memory

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

    return 0;
}

[Bogeyman]

Thanks coder777.

I'm not sure if I'm doing something wrong, but the index calculation you provided doesn't seem to access all the elements either. It appears to access the elements [0], [1], [2], [2], [3], [4] when row = 2 and col = 3. There is repeated access at element [2] and the sixth element is never accessed. Like I said, I might be doing something wrong, but I can't figure out what that might be.

This is the loop:

int counter = 0;
    for(i = 0; i < row; ++i)
    {
        for(j = 0; j < col; ++j)
        {
            *(grid + (i * (col - 1)) + j) = counter;// elements 0, 1, 2, 2, 3, 4
            counter++;
        }
    }

(Note that I changed the pointer to type int and allocated int sized blocks of memory with new[] to demonstrate value incrementation.)

I didn't know you could copy objects directly like that. Does copying map1 to map2 give map2 all of map1's members in their current state?

How can you avoid the problem of copied objects trying to de-allocate the same memory? Do you just have to avoid using pointers when copying objects?

[coder777]

You are right, I should have tested it. The real correct is without - 1:

*(grid + (i * col) + j)

Does copying map1 to map2 give map2 all of map1's members in their current state?

Yes.

How can you avoid the problem of copied objects trying to de-allocate the same memory?

You need your own copy constructor/operator:

class Map
{
    private: // The compiler will raise an error on copy
        Map(const Map &);
        void operator=(const Map &);
    public:
        Map();
...
};

[MikeyBoy]

A few things:

1) What's your use case for the default constructor? Why do you need it at all? Is that ever going to be a valid state for objects of this class?

2) I was going to mention copy constructors and assignment operators, but coder777 got there first. Have you come across the Rule of Three? In general, any time you need to worry about writing a destructor, that's usually a sign that you need to worry about dealing with a copy constructor, and an assignment operator, too.

3) While your constructor prints out a message to the user if memory allocation fails, you've explicitly disabled exception throwing from the memory allocation. This means your calling code has no way of knowing the allocation has failed, which could lead you to a state of having non-zero row and col. This would lead to undefined behaviour in draw() and display().

4) What is the point of the data members i and j? It looks as though you're only using them as counters for local loops, so what's the point of making them members?

[Bogeyman]

This code is just me playing around with ideas for the class. Nothing is finalised and plenty is flawed. It's all about the learning experience.

1) It's not in the code, but one of the things I was thinking of doing was allocating a default block of memory for the array if no dimensions were specified. Currently the constructor just sets the pointer to NULL. I may end up scrapping the idea and the default constructor.

2) I have not come across the Rule of Three, copy constructors or assignment operators. These are completely new concepts to me.

3) Yet another thing I haven't yet covered are exceptions and exception handling, that's why(for now) I've used the

nothrow

version of new[].

4) Yes, they are only used as loop counters, but what's wrong with making them member variables? This way they don't need to be re-declared every time I may want to use them in a new function.

I would appreciate it if you would be willing to try to explain these concepts to me, though I fully understand if you're not. There's a lot there to try to explain I'm sure. As it is I'm looking at tutorials online, but it's tough going as many language concepts are not explained very clearly.

[MikeyBoy]

1) It's not in the code, but one of the things I was thinking of doing was allocating a default block of memory for the array if no dimensions were specified.

I'm not sure there's much point doing that. You wouldn't use that memory until you assign row and column sizes, at which point you'd probably have to reallocate the memory anyway.

If there's no need for a default constructor, there's no harm in doing away with it completely.

2) I have not come across the Rule of Three, copy constructors or assignment operators. These are completely new concepts to me.

Copy constructors are worth looking at. Basically, they're just a type of constructor that takes, as its argument, an existing object of the same type. The idea is that you're constructing an object that is initialised in such a way that it's a copy of another object. There's nothing particularly advanced about it.

Overloading operators is a slightly more advanced topic, and you can probably wait until you reach it as part of your normal learning. Basically, I'm talking about creating a special version of the "=" operator that works for whole objects.

3) Yet another thing I haven't yet covered are exceptions and exception handling, that's why(for now) I've used the

nothrow

version of new[].

Fair enough. Exception handling is definitely an advanced topic, so don't worry about it now. Although really, my point was that you have a scenario in which your draw() and display() methods are unsafe, because they could be called with non-zero row and column, but a NULL grid pointer.

4) Yes, they are only used as loop counters, but what's wrong with making them member variables? This way they don't need to be re-declared every time I may want to use them in a new function.

As a general rule, you should keep variables/objects to as limited a scope as possible. This has several advantages:

- it makes it clear what the lifespan of any given variable is, and where its value might be used
- it prevents coupling between different parts of code that have no need to be coupled
- it stops you accidentally using a value that's still stored in the variable from a previous, unrelated use

As it is I'm looking at tutorials online, but it's tough going as many language concepts are not explained very clearly.

I know the lure of free online stuff is strong but, really, you can't overstate the importance of a good, well-written book written by a professional writer who (a) knows their subject, and (b) knows how to present it clearly to their reader.

Without wishing to belittle the efforts of tutorial writers, online tutorials are often written by enthusiasts, in their spare time. They're very unlikely ever to give you teaching as good as that which you'll get from a dedicated professional.

[Bogeyman]

I tried Map map2 = map1; without providing a copy constructor and the program didn't crash. Why is that?

If an object copied this way shares the same pointers as the object it was copied from, does that mean that any modifications made to the contents of one objects pointers will affect the other?

Considering that the program didn't crash, does that mean that de-allocating the same memory twice - once in each objects destructor - is not guaranteed to make a program crash, instead resulting in undefined behaviour?

[ne555]

I doubt that there is anything guaranteed to crash, all lives in the realm of undefined behaviour.

[MikeyBoy]

If an object copied this way shares the same pointers as the object it was copied from, does that mean that any modifications made to the contents of one objects pointers will affect the other?

Yes, that's exactly what will happen. The pointer in map2 will be a copy of the pointer in map1 - both pointing to the same area of memory.

Considering that the program didn't crash, does that mean that de-allocating the same memory twice - once in each objects destructor - is not guaranteed to make a program crash, instead resulting in undefined behaviour?

Yes, exactly - undefined behaviour. It doesn't surprise me that an implementation might be resilient to trying to free the same bit of memory twice. 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.