3

I'm getting a really weird error where after I leave the for scope I can't access whatever my pointer was pointing during the loop even if the array holding the objects is declared in the class header.

This is the basic of the code:

Class CTile{ /*Code*/ };

Class CMap  
{  
    public:  
        CTile** tiles;  
        CMap();  
}

CMap::CMap()  
{  
    int lines = 10;
    int cols = 10;
    tiles = new CTile*[lines];  
    for(int i = 0 ; i (lower than) lines;++)  
    {  
        this->tiles[i] = new CTile[cols];  
    }  
    for(int curLine = 0; curLine (lower than) lines ; curLine++)  
        for(int curCol = 0; curCol (lower than) cols; curCol++)  
        {
            CTile me = this->tiles[curLine][curCol];
            me.setType(1);
            //do whatever I need, and inside the loop everything works.  
        }  
    int a = this->tiles[2][2].getType(); // a gets a really weird number 
    this->tiles[2][2].setType(10); // crashes the program

}

Does anyone know what could be wrong?

Luke B.
  • 1,258
  • 1
  • 17
  • 28
  • One thing I notice is that you never seem to be initializing the columns in the tiles double array. (This is from your first for loop) You create the new CTile[cols] but never create an object inside of those. – Jaime Garcia May 06 '11 at 16:23
  • Off topic, but you realize that `this->` is completely redundant? – Mark Ransom May 06 '11 at 16:32
  • @Mark: I personally like it, makes the classes own attributes / functions stand-out in the methods. – Matthieu M. May 06 '11 at 16:36
  • @Luke B: you realize that a `std::vector< std::vector >` would make much of this code completely trivial of course, and only use those pointers because you like debugging nightmare and reinventing the wheel... I hope ? – Matthieu M. May 06 '11 at 16:37
  • @Matthieu, I've seen that reasoning used before. Personally I prefer a naming convention that makes it clear which variables are members and which are not. – Mark Ransom May 06 '11 at 17:03
  • @Mark: I also like a naming convention that makes it clear (for attributes) and generally uses the `_` (at work) or the `m` prefix at home. To distinguish methods from functions though, I just use `this`. – Matthieu M. May 06 '11 at 17:37
  • @Matthieu I just find it easier to add new values to a multidimensional array than to a vector of vector, unless there's an easier way besides declaring a new vector and pushing it back inside the first. – Luke B. May 06 '11 at 18:45
  • @Mark yes, I know `this->` is redundant, I prefer that over using prefixes on everything – Luke B. May 06 '11 at 18:46
  • @Luke B: I don't quite follow you, `CMap::CMap(): tiles(10, std::vector(10, 1)) {}` is pretty much clearer that your three for loops. I don't even need an answer to write it! – Matthieu M. May 06 '11 at 18:54

2 Answers2

4
CTile me = this->tiles[curLine][curCol];

That should be

CTile& me = this->tiles[curLine][curCol];
me.setType(1);

Why? Because you made a copy of CTile, instead of creating a reference to the one in the 2-dimensional array. You might now discover that the crash has moved to the me.setType(1) statement.

Andy Finkenstadt
  • 3,547
  • 1
  • 21
  • 25
  • As an aside, because you aren't using a naming convention for member variables (such as mTiles), I appreciate that you are using the explicit `this->tiles` to emphasize that `tiles` *is* a member variable. The syntax highlighters will bring it to your eye's attention faster than your brain will remember that 'tiles' is not a local variable or parameter given to a function. – Andy Finkenstadt May 06 '11 at 16:29
  • That makes sense and the code is working now, thanks a lot :) – Luke B. May 06 '11 at 16:40
4
CTile  me = this->tiles[curLine][curCol];

Here is the problem. me is a copy of the original object tiles[curLine][curCol], so whatever you're doing with me isn't reflected in the original object. The orginal object remains unchanged even if you do me.setType(1). I'm sure you didn't want that.

So the fix is : use reference as :

CTile & me = this->tiles[curLine][curCol];
  //  ^ note this
me.setType(1);

Or better yet, you can simply do this:

tiles[curLine][curCol].setType(1); //"this" is implicit!
Nawaz
  • 353,942
  • 115
  • 666
  • 851