I think I have a memory leak?

Question

I am trying to do the Knight's Tour using dynamic arrays because the user will be able to implement their own board size, and where they want to start the knight at in the board. However, before I changed everything to dynamic arrays, I was able to perform my code just find using static arrays, but now that I switched to dynamic arrays whenever my code executes, I get a memory leak I believe and the program crashes. I was wondering if my deconstructor is not working properly? Or if there is a different way I have to delete the dynamic array? I am also wondering if there is a way to make this code a little more efficient, more exactly make the Move() function a little more efficient. Thanks.

#include <iostream>
#include <iomanip>
#include "stdafx.h"
using namespace std;

class Knight
{
private:
int Tracker = 1;
int BoardSize;
int **Board = new int*[BoardSize];
public:
Knight(int s)
{
    BoardSize = s;
    for (int i = 0; i <= s -1; i++)
    {
        Board[i] = new int[s];
    }
    for (int i = 0; i <= s - 1; i++)
    {
        for (int j = 0; j <= s - 1; j++)
        {
            Board[i][j] = 0;
        }
    }
}
~Knight()
{
    for (int i = 0; i <= BoardSize - 1; i++)
    {
        delete[] Board[i];
    }
    delete[] Board;
}

void MarkUp(int &val)
{
    val = Tracker;
    Tracker++;
}

void MarkDown(int &val)
{
    val = 0;
    Tracker--;
}

bool PossibleMove(int &val)
{
    if (val == 0)
        return 1;
    else
        return 0;
}

void Display()
{
    for (int k = 0; k < (BoardSize * 5) + 1; k++)
    {
        cout << "-";
    }
    cout << endl;
    for (int i = 0; i <= BoardSize - 1; i++)
    {
        cout << "| ";
        for (int j = 0; j <= BoardSize - 1; j++)
        {
            cout << setw(2) << setfill('0') << Board[i][j] << " | ";
        }
        cout << endl;
        for (int k = 0; k < (BoardSize * 5) + 1; k++)
        {
            cout << "-";
        }
        cout << endl;
    }
    cout << endl << endl;
}

bool Move(int x, int y)
{
    if (Tracker > (BoardSize * BoardSize))
    {
        return true;
    }
    if (PossibleMove(Board[x][y])) {
        if ((x - 2 >= 0) && (y + 1 <= (BoardSize - 1)))
        {
            MarkUp(Board[x][y]);
            if (Move(x - 2, y + 1))
            {
                return true;
            }
            else
            {
                MarkDown(Board[x][y]);
            }
        }
        if ((x - 2 >= 0) && (y - 1 >= 0))
        {
            MarkUp(Board[x][y]);
            if (Move(x - 2, y - 1))
            {
                return true;
            }
            else
            {
                MarkDown(Board[x][y]);
            }
        }
        if ((x - 1 >= 0) && (y + 2 <= (BoardSize - 1)))
        {
            MarkUp(Board[x][y]);
            if (Move(x - 1, y + 2))
            {
                return true;
            }
            else
            {
                MarkDown(Board[x][y]);
            }
        }
        if ((x - 1 >= 0) && (y - 2 >= 0))
        {
            MarkUp(Board[x][y]);
            if (Move(x - 1, y - 2))
            {
                return true;
            }
            else
            {
                MarkDown(Board[x][y]);
            }
        }
        if ((x + 2 <= (BoardSize - 1)) && (y + 1 <= (BoardSize - 1)))
        {
            MarkUp(Board[x][y]);
            if (Move(x + 2, y + 1))
            {
                return true;
            }
            else
            {
                MarkDown(Board[x][y]);
            }
        }
        if ((x + 2 <= (BoardSize - 1)) && (y - 1 >= 0))
        {
            MarkUp(Board[x][y]);
            if (Move(x + 2, y - 1))
            {
                return true;
            }
            else
            {
                MarkDown(Board[x][y]);
            }
        }
        if ((x + 1 <= (BoardSize - 1)) && (y + 2 <= (BoardSize - 1)))
        {
            MarkUp(Board[x][y]);
            if (Move(x + 1, y + 2))
            {
                return true;
            }
            else
            {
                MarkDown(Board[x][y]);
            }
        }
        if ((x + 1 <= (BoardSize - 1)) && (y - 2 >= 0))
        {
            MarkUp(Board[x][y]);
            if (Move(x + 1, y - 2))
            {
                return true;
            }
            else
            {
                MarkDown(Board[x][y]);
            }
        }
    }
    return false;
}   
};

int main()
{
int size = 0;
int Row, Col;
int opt = 0;

do
{
    cout << "Welcome to Knights Tour!" << endl;
    cout << "1) Start the Tour." << endl;
    cout << "2) Quit." << endl;
    cin >> opt;

    switch (opt)
    {
    case 1:
    {
        cout << "Enter board size:" << endl;
        cin >> size;

        Knight K1(size);

        cout << "Enter Row:" << endl;
        cin >> Row;
        cout << "Enter Column: " << endl;
        cin >> Col;

        if (K1.Move(Row, Col))
        {
            cout << "\nOperation was Successful." << endl;
            cout << "Possible Solution:" << endl;
            K1.Display();
        }
        else
        {
            cout << "\nThat is not Possible." << endl;
        }
        cout << endl;
        break;
    }
    case 2:
    {
        exit(0);
        break;
    }
    default:
    {
        cout << "Not a Valid Option." << endl;
        cout << "Try Again Please." << endl;
        cout << endl;
        break;
    }
    }
} while (opt != 2);
return 0;
}

Why not use [std::vector](http://en.cppreference.com/w/cpp/container/vector)? — puelo, Oct 20 '17 at 14:14
How long into the execution does your program crash? Memory leaks rarely cause a crash unless your system absolutely runs out of memory. Please share the error message you get when your application fails. — François Andrieux, Oct 20 '17 at 14:15
"_I get a memory leak I believe and the program crashes_" Memory leaks do not cause crashes. They just leak memory. — Algirdas Preidžius, Oct 20 '17 at 14:17
Have you tried running this in a debugger? Also, I'm not sure, but the code concerning `Knight`'s `Board` variable looks odd to me. You allocate int arrays to elements of `Board` in the constructor, but never assign anything to `Board` itself in the constructor. I.e. seems to me `Board` is only assigned something after the constructor and you write/read to memory that you didn't allocate. — AVH, Oct 20 '17 at 14:18
Just a side note - your naming convention is really inconsistent. Once you declare a variable with PascalCase, another time you declare a variable with camelCase. Stick to one and use it accross all your project. If the code was more complex than this, it would be nearly unreadable. — ProXicT, Oct 20 '17 at 14:35

score 3 · Answer 1 · answered Oct 20 '17 at 14:21

This code does not work as you think it does:

int BoardSize;
int **Board = new int*[BoardSize];

The value of BoardSize is going to be whatever happens to be in the memory that gets allocated for it, so the new is going to try to allocate an unknowable size array.

Don't use hand coded dynamic arrays for this. Use std::vector; That's what it's for. In real life production code, you should almost never ever use dynamically allocated arrays. You'll use one of the standard containers.

AVH · Answer 2 · 2017-10-20T14:44:43.273

1

As noted in the comments by multiple people, memory leaks do not cause crashes (generally).

The issue is that inside the constructor you assign values to the Board array elements. However, you never allocate memory for the Board array itself. I.e. when you do Board[i] = new int[s];, Board points to some random address. This is because the line int **Board = new int*[BoardSize]; is not executed before the constructor starts.

So the following should work:

class Knight {
  private:
    int BoardSize;
    int **Board;

  public:
    Knight (int s) {
      BoardSize = s;
      Board = new int*[BoardSize];

      // Remainder of code
    }
};

However, I really suggest you use std::vector for this instead. Then you won't have to deal with memory (de)allocation at all. That could look as concise as the following:

#include <vector>

class Knight {
  private:
    int BoardSize;
    std::vector<std::vector<int>> Board;

  public:
    Knight (int s) : BoardSize(s), Board(s, std::vector<int>(s, 0)) { }
};

Note that I initialized the class members directory in the member initializer list.

edited Oct 20 '17 at 14:44

answered Oct 20 '17 at 14:21

AVH

11,349
4
34
43

std::vector allows you to stop worrying about memory *deallocation*. You still need to resize the board to `s` columns, and resize each column to `s` rows (or the other way round - I can never remember). – Martin Bonner supports Monica Oct 20 '17 at 14:24
@MartinBonner Oh, sure, but you don't get "exposed" to the allocations either, you just tell vector how many elements you want it to contain. – AVH Oct 20 '17 at 14:39
Thank you Darhuuk that fixed it. although I do not understand why I can't have it how I originally did because isn't it essentially doing the same thing still? – Cavustius Oct 20 '17 at 14:41
@MichaelHeck I expanded my answer a little. Is it clear now? – AVH Oct 20 '17 at 14:44
Yes Darhuuk that explained a lot. Thank you very much. – Cavustius Oct 22 '17 at 02:34
Now I am having an issue with a bigger board, it just sits there and does not load. I am thinking it is due to it not being able to get over my if statements to move, and I am thinking I am going to need a function that checks for every possible move the board can make, and if that function returns true, then I will need to break out of the loops and print a statement that says it is not possible...however I am unsure of how to implement that? – Cavustius Oct 22 '17 at 02:35
@Cavustius If you have extra issues and can't figure it out with a debugger, simply post a new question. – AVH Oct 22 '17 at 22:17

I think I have a memory leak?

2 Answers2