Possible segmentation fault: Am I using the "this->" operator correctly?

Question

I am doing a homework problem that I have a question about. If you don't feel comfortable assisting with a homework problem, I should say that my instructor has encouraged us to ask for help on this site when we are completely stumped. Also, I have completed the basic portion of the assignment on my own, and am now doing an optional challenge problem. Anyway, on to the problem!

Being new to OOP and C++ in general, I am having trouble understanding the "this->" operator. We haven't covered it in class, but I have seen it elsewhere and I am sort-of guessing how it is meant to be used.

For the assignment, I have to create a console based Tic-Tac-Toe game. Only the challenge portion of the assignment wants us to create an AI opponent, and we don't get any extra credit for doing the challenge, I just want to know how to do it. I am studying things like minimax and game trees, but for now I just wanted to create a "pick a random, open spot" function.

I have a class called TicTacToe which is basically the entire program. I will post it below with the parts that are relevant to the question, but part that is giving me an error is this subroutine:

void TicTacToe::makeAutoMove(){
    srand(time(NULL));
    int row = rand() % 3 + 1;
    int col = rand() % 3 + 1;
    if(this->isValidMove(row, col)){
        this->makeMove(row, col);
    }else{
        this->makeAutoMove();
    }
}

The only thing that this function is meant to do is make a move on the board, assuming that it is open. The board is set up like:

char board[4][4];

and when I print it, it looks like:

   1  2  3
1  -  -  - 
2  -  -  -
3  -  -  -

The problem, is that on occasion a move is made by the computer that gives me an error that is difficult to track down because of the random nature of the function. I believe it is a segfault error, but I can't tell because I can't replicate it in my debugger.

I think that the "this->" operator functions as a pointer, and if a pointer is NULL and it is accessed it could give me this problem. Is this correct? Is there a way to fix this?

I understand that this may be a very low-level question to many of the members of the community, but I would appreciate your help as long as it doesn't come with snide remarks about how trivial this is, or how stupid I must be. I'm LEARNING, which means that I am going to have some silly questions sometimes.

Here is more of my .cpp file if it helps:

TicTacToe::TicTacToe()
{
    for(int row = 0; row < kNumRows; row++){
        for(int col = 0; col < kNumCols; col++){
            if(col == 0 && row == 0){
                board[row][col] = ' ';
            }else if(col == 0){
                board[row][col] = static_cast<char>('0' + row);
            }else if(row == 0){
                board[row][col] = static_cast<char>('0' + col);
            }else{
                board[row][col] = '-';
            }
        }
    }
    currentPlayer = 'X';
}

char TicTacToe::getCurrentPlayer(){
    return currentPlayer;
}

char TicTacToe::getWinner(){
    //Check for diagonals (Only the middle square can do this)
    char middle = board[2][2];
    if(board[1][1] == middle && board[3][3] == middle && middle != '-'){
        return middle;
    }else if(middle == board[3][1] && middle == board[1][3] && middle != '-'){
        return middle;
    }

    //Check for horizontal wins
    for(int row = 1; row < kNumRows; row++){
        if(board[row][1] == board[row][2] && board[row][2] == board[row][3] && board[row][1] != '-'){
            return board[row][1];
        }
    }

    //Check for vertical wins
    for(int col = 1; col < kNumCols; col++){
        if(board[1][col] == board[2][col] && board[2][col] == board[3][col] && board[1][col] != '-'){
            return board[1][col];
        }
    }

    //Otherwise, in the case of a tie game, return a dash.
    return '-';
}

void TicTacToe::makeMove(int row, int col){
    board[row][col] = currentPlayer;
    if(currentPlayer == 'X'){
        currentPlayer = 'O';
    }else if(currentPlayer == 'O'){
        currentPlayer = 'X';
    }
}

//TODO: Make sure this works after you make the make-move function
bool TicTacToe::isDone(){
    bool fullBoard = true;
    //First check to see if the board is full
    for(int col = 1; col < kNumCols; col++){
        for(int row = 1; row < kNumRows; row++){
            if(board[row][col] == '-'){
                fullBoard = false;
            }
        }
    }

    //If the board is full, the game is done. Otherwise check for consecutives.
    if(fullBoard){
        return true;
    }else{
        //Check for diagonals (Only the middle square can do this)
        char middle = board[2][2];
        if(board[1][1] == middle && board[3][3] == middle && middle != '-'){
            return true;
        }else if(middle == board[3][1] && middle == board[1][3] && middle != '-'){
            return true;
        }

        //Check for horizontal wins
        for(int row = 1; row < kNumRows; row++){
            if(board[row][1] == board[row][2] && board[row][2] == board[row][3] && board[row][1] != '-'){
                return true;
            }
        }

        //Check for vertical wins
        for(int col = 1; col < kNumCols; col++){
            if(board[1][col] == board[2][col] && board[2][col] == board[3][col] && board[1][col] != '-'){
                return true;
            }
        }
    }
    //If all other tests fail, then the game is not done
    return false;
}

bool TicTacToe::isValidMove(int row, int col){
    if(board[row][col] == '-' && row <= 3 && col <= 3){
        return true;
    }else{
        //cout << "That is an invalid move" << endl;
        return false;
    }
}

void TicTacToe::print(){
    for(int row = 0; row < kNumRows; row++){
        for(int col = 0; col < kNumCols; col++){
            cout << setw(3) << board[row][col];
        }
        cout << endl;

    }
}

Answer 1

A general preface: you almost never need to use this explicitly. In a member function, in order to refer to member variables or member methods, you simply name the variable or method. As with:

class Foo
{
  int mN;
public:
  int getIt()
  {
    return mN; // this->mN legal but not needed
  }
};

I think that the "this->" operator functions as a pointer, and if a pointer is NULL and it is accessed it could give me this problem. Is this correct? Is there a way to fix this?

this is a pointer, yes. (Actually, it's a keyword.) If you call a non-static member function of a class, this points to the object. For instance, if we were to call getIt() above:

int main()
{
  Foo f;
  int a = f.getIt();
}

then this would point to f from main().

Static member functions do not have a this pointer. this cannot be NULL, and you cannot change the value of this.

There are several cases in C++ where using this is one way to solve a problem, and other cases where this must be used. See this post for a list of these situations.

Answer 2

I could reproduce the bug on coliru's g++4.8.1 when not compiling with optimizations. As I said in a comment, the problem is the srand combined with time and the recursion:

The return value of time is often the Unix time, in seconds. That is, if you call time within the same second, you'll get the same return value. When using this return value to seed srand (via srand(time(NULL))), you'll therefore set the same seed within this second.

void TicTacToe::makeAutoMove(){
    srand(time(NULL));
    int row = rand() % 3 + 1;
    int col = rand() % 3 + 1;
    if(this->isValidMove(row, col)){
        this->makeMove(row, col);
    }else{
        this->makeAutoMove();
    }
}

If you don't compile with optimizations, or the compiler otherwise needs to use stack space to do an iteration of makeAutoMove, each call will occupy a bit of the stack. Therefore, when called often enough, this will produce a Stack Overflow (luckily, you went to the right site).

As the seed doesn't change within the same second, the calls to rand will also produce the same values within that second - for each iteration, the first rand will always produce some value X and the second always some value Y within that second.

If X and Y lead to an invalid move, you'll get infinite recursion until the seeding changes. If your computer is fast enough, it might call makeAutoMove often enough to occupy enough stack space within that second to cause a Stack Overflow.

---

Note that it's not required to seed the Pseudo-Random Number Generator used by rand more than once. Typically, you do only seed once, to initialize the PRNG. Subsequent calls to rand then produce pseudo-random numbers.

From cppreference:

Each time rand() is seeded with srand(), it must produce the same sequence of values.

cppreference: rand, srand

Answer 3

Here is the first pass:

1. Arrays start counting from zero. So you do not need the +1 in lines like rand() % 3 + 1;
2. Indeed this is a point to the current object. Usually you do not need to use it. i.e. this->makeMove(row, col); and makeMove(row, col); work the same
3. char board[4][4];1 should bechar board[3][3];` as you want a 3x3 board. See 1) above
4. board[row][col] = static_cast<char>('0' + row); - You do not need the static cast '0' + row will suffice
5. You need to take account of (1) in the rest of your code
6. If you get segmentation problems it is best to use the debugger. A very skill to learn

Anyway - Good luck with your studies. It is refreshing to get a new poster on this web site that is keen to learn

Answer 4

Just a side note about recursion, efficiency, robust coding and how being paranoid can help.

Here is a "cleaned up" version of your problematic function.
See other answers for explanations about what went wrong with the original.

void TicTacToe::makeAutoMove() {

    // pick a random position
    int row = rand() % 3;
    int col = rand() % 3;

    // if it corresponds to a valid move
    if (isValidMove(row, col)){
        // play it
        makeMove(row, col);
    }else{
        // try again
        makeAutoMove(); // <-- makeAutoMove is calling itself
    }
}

Recursion

In plain English you could describe what the code does as:

• pick a random (row, col) couple.
• if this couple represents a valid move position, play that move
• else try again

Calling makeAutoMove() is indeed a very logical way of trying again, but a not so efficient one programming-wise.

Each new call will cause some memory allocation on the stack:

• 4 bytes for each local variable (8 bytes in total)
• 4 bytes for the return address

So the stack consumption will look like:

makeAutoMove             <-- 12 bytes
    makeAutoMove         <-- 24
        makeAutoMove     <-- 36
            makeAutoMove <-- 48
                         <-- etc. 

Imagine for a second that you inadvertently call this function in a situation where it cannot succeed (when a game has ended and no more valid moves are available).

The function will then call itself endlessly. It will be only a matter of time before stack memory gets exhausted and the program crashes. And, given the computing power of your average PC, the crash will occur in the blink of an eye.

This extreme case illustrates the (hidden) cost of using recursive calls. But even if the function eventually succeeds, the cost of each retry is still there.

The things we can learn from there:

• recursive calls have a cost
• they can lead to crashes when the termination conditions are not met
• a lot of them (but not all of them) can easily be replaced by loops, as we will see

As a side note within the side note, as dyp duly noted, modern compilers are so smart they can, for various reasons, detect some patterns within the code that allow them to eliminate such kind of recursive calls.
Nevertheless, you never know if your particular compiler will be smart enough to remove banana peels from under your sloppy feets, so better avoid slopiness altogether, if you ask me.

Avoiding recursion

To get rid of that naughty recursion, we could implement the try again like so:

void TicTacToe::makeAutoMove() {
try_again:
    int row = rand() % 3;
    int col = rand() % 3;
    if (isValidMove(row, col)){
        makeMove(row, col);
    }else{
        goto try_again; // <-- try again by jumping to function start
    }
}

After all, we don't really need to call our function again. Jumping back to the start of it will be enough. That's what the goto does.

Good news is, we got rid of the recursion without changing much of the code.
Not so good news is, we used an ugly construct to do so.

Preserving regular program flow

We don't want to keep that ungainly goto since it breaks the usual control flow and makes the code very difficult to understand, maintain and debug ^*.

We can, however, replace it easily with a conditional loop:

void TicTacToe::makeAutoMove() {

    // while a valid move has not been found
    bool move_found = false;
    while (! move_found) {

        // pick a random position
        int row = rand() % 3;
        int col = rand() % 3;

        // if it corresponds to a valid move
        if (isValidMove(row, col)){
            // play it
            makeMove(row, col);
            move_found = true; // <-- stop trying
        }
    }
}

The good: bye bye Mr goto
The bad : hello Mrs move_found

Keeping the code sleek

We swapped the goto for a flag.
It's already better (the program flow is not broken anymore), but we have added some complexity to the code.

We can relatively easily get rid of the flag:

    while (true) { // no way out ?!?

        // pick a random position
        int row = rand() % 3;
        int col = rand() % 3;

        // if it corresponds to a valid move
        if (isValidMove(row, col)){
            // play it
            makeMove(row, col);
            break; // <-- found the door!
        }
    }
}

The good: bye bye Mrs move_found
The bad : we use a break, that is little more than a tamed goto (something like "goto the end of the loop").

We could end the improvements there, but there is still something annoying with this version: the exit condition of the loop is hidden within the code, which makes it more difficult to understand at first glance.

Using explicit exit conditions

Exit conditions are especially important to figure whether a piece of code will work or not (the reason why our function gets stuck forever is precisely that there are some cases where the exit condition is never met).

So it's always a good idea to make exit conditions stand out as clearly as possible.

Here is a way to make the exit condition more apparent:

void TicTacToe::makeAutoMove() {

    // pick a random valid move
    int row, col;
    do {
        row = rand() % 3;
        col = rand() % 3;
    } while (!isValidMove (row, col)); // <-- if something goes wrong, it's here

    // play it
    makeMove(row, col);
}

You could probably do it a bit differently. It does not matter as long as we achieve all of these goals:

• no recursion
• no extraneous variables
• meaningful exit condition
• sleek code

When you compare the latest refinement with the original version, you can see that it has mutated to something significantly different.

Code robustness

As we have seen, this function can never succeed in case no more legal moves are available (i.e. the game has ended).
This design can work, but it requires the rest of your algorithm to make sure end game conditions are properly checked before this function is called.

This makes your function dependent on external conditions, with nasty consequences if these conditions are not met (a program hangup and/or crash).

This makes this solution a fragile design choice.

Paranoia to the rescue

You might want to keep this fragile design for various reasons. For instance, you might prefer to wine & dine your g/f rather than dedicating your evening to software robustness improvements.

Even if your g/f eventually learns how to cope with geeks, there will be cases when the best solution you can think of will have inherent potential inconsistencies.

This is perfectly OK, as long as these inconsistencies are spotted and guarded against.

A first step toward code robustness is to make sure a potentially dangerous design choice will be detected, if not corrected altogether.

A way of doing so is to enter a paranoid state of mind, imagining that every system call will fail, the caller of any of your function will do its best to make it crash, every user input will come from a rabid Russian hacker, etc.

In our case, we don't need to hire a rabid Russian hacker and there is no system call in sight. Still, we know how an evil programmer could get us in trouble, so we will try to guard against that:

void TicTacToe::makeAutoMove() {

    // pick a random valid move
    int row, col;

    int watchdog = 0; // <-- let's get paranoid

    do {
        row = rand() % 3;
        col = rand() % 3;

        assert (watchdog++ < 1000); // <-- inconsistency detection

    } while (!isValidMove (row, col));

    // play it
    makeMove(row, col);
}

assert is a macro that will force a program exit if the condition passed as parameter is not met, with a console message and/or popup window saying something like assertion "watchdog++ < 1000" failed in tictactoe.cpp line 238.
You can see it as a way to bail out of a program if a fatal algorithmic flaw (i.e. the kind of flaw that will require a source code overhaul, so there is little point in keeping this inconsistent version of the program running) has been detected.

By adding the watchdog, we make sure the program will explicitely exit if it detects an abnormal condition, indicating gracefully the location of the potential problem (tictactoe.cpp line 238 in our case).

While refactoring your code to eliminate inconsistencies can be difficult or even impossible, detecting inconsistencies is most often very easy and cheap.

The condition have not to be very precise, the only point is to make sure your code is executing in a "reasonably" consistent context.

In this example, the actual number of trials to get a legit move is not easy to estimate (it's based on cumulative probabilities to hit a cell where a move is forbidden), but we can easily figure that failing to find a legit move after 1000 tries means something went seriously wrong with the algorithm.

Since this code is just there to increase robustness, it does not have to be efficient. It's just a means to go from the "why the hell does my program hang?!?" situation to the "dang, I must have called makeAutoMove after end game" (near) immediate realization.

Once you've tested and proved your program, and if you have really good reasons for that (namely, if your paranoid checks cause serious performance issues) you can take the decision to cleanup that paranoid code, leaving very explicit comments in your source about the way this particular piece of code shall be used.

Actually there are means to keep the paranoid code live without sacrificing efficiency, but that's another story.

What it boils down to is:

• get used to notice potential inconsistencies in your code, especially when these inconsistencies can have serious consequences
• try to make sure as many pieces of your code as possible can detect inconsistencies
• sprinkle your code with paranoid checks to increase your chances of detecting wrong moves early

Code refactoring

In an ideal world, each function should give a consistent result and leave the system in a consistent state. That rarely happens in real life, unless you accept some limitations to your creativity.

However, it could be interesting to see what you could achieve if you designed a tic-tac-toe 2.0 with these guidelines in mind. I'm sure you would find a lot of helpful reviewers here on StackOverflow.

Feel free to ask if you found some points of interest in all these rants, and welcome to the wonderful world of geeks :)
(kuroi dot neko at wanadoo dot fr)

---

* goto might look harmless enough in such a small example, but you can trust me on this: abusing goto will lead you to a world of pain. Just don't do it unless you have a very, very good reason.