C / C++ / MFC

28-Sep-08 19:14

In one of my projects I have an array of pointers to Tile's (a class of my own) within a Block class. Upon calling the constructor for the Block class I assign the pointer to each Tile in the array to the address of a Tile created on the heap using the new operator like so;

Block::Block()
{
while(i=0; i < 20; i++)
{
Array_of_pointers_to_Tiles[i] = new Tile;
}
}

I then call the delete operator in the destructor like so;

Block::~Block()
{
while(i=0; i < 20; i++)
{
delete Array_of_pointers_to_Tiles[i];
Array_of_pointers_to_Tiles[i] = NULL;
}
}

This code does not compile.

According to this resource[^] I am to allocate the array like so;

int* a = NULL;   // Pointer to int, initialize to nothing.
int n;           // Size needed for array
cin >> n;        // Read in the size
a = new int[n];  // Allocate n ints and save ptr in a.
for (int i=0; i<n; i++) {
    a[i] = 0;    // Initialize all elements to zero.
}
. . .  // Use a as a normal array
delete [] a;  // When done, free memory pointed to by a.
a = NULL;     // Clear a to prevent using invalid memory reference.

and to free the memory allocated to the array;

delete [] a;  // Free memory allocated for the a array.
a = NULL;     // Be sure the deallocated memory isn't used.

However this code doesn't compile either! Frown | :(

What is the correct way?!?

modified on Monday, September 29, 2008 1:44 AM

Saurabh.Garg28-Sep-08 19:23

Saurabh.Garg

28-Sep-08 19:23

Can you tell what is the compiler error?

Saurabh

Sauce!28-Sep-08 19:28

28-Sep-08 19:28

As soon as I get home, yes Poke tongue | ;-P

I'm at work Laugh | :laugh:

Sauce!29-Sep-08 21:01

29-Sep-08 21:01

Okay, so it turns out that I was accessing each array element using the points-to operator instead of the dot-operator :P

You can't use the points-to operator in this situation because when you think about it, it doesn't make any sense to :P

EDIT: When running in debug mode I get a debug assertion failure. Attempting to debug produces this:

"0 > 8828 Main Thread Main Thread Tile::`vector deleting destructor' Normal 0"

on the line delete [] p_TileArray;

What's going on? Frown | :(

modified on Tuesday, September 30, 2008 3:08 AM

Iain Clarke, Warrior Programmer30-Sep-08 0:20

30-Sep-08 0:20

Unless we see how you defined the array (from your Block class definition), we're not going to be able to help...

Iain.

Sauce!30-Sep-08 0:55

30-Sep-08 0:55

In Block.h

class Block
{
public:
	Block(void);
	Block(int itsShape, int itsColour);
	~Block(void);

	bool isGrounded(void);
	void MoveLeft();
	void MoveRight();
	void MoveDown();
	void Rotate();

	void Render();

private:
	int Shape;
	int Colour;
	bool Grounded;

	Tile* p_TileArray;
};

in Block.cpp

#include "Block.h"

Block::Block(void)
{
	Grounded = false;
	Shape = (rand() % 7);
	Colour = (rand() % 6);

	p_TileArray = NULL;
	p_TileArray = new Tile[4];

	p_TileArray[0].SetPos(498.0, 82.0);
	p_TileArray[1].SetPos(530.0, 82.0);
	p_TileArray[2].SetPos(498.0, 114.0);
	p_TileArray[3].SetPos(530.0, 114.0);
}

Block::~Block(void)
{
	if (p_TileArray)
	{
		delete [] p_TileArray; //Debug assertion failure / 'vector deleting destructor'
		p_TileArray = NULL;
	}
}

bool Block::isGrounded(void)
{
	return Grounded;
}
void Block::MoveLeft()
{
	if(Grounded == false)
	{
		for(int i = 0; i < (sizeof(p_TileArray) / sizeof(p_TileArray[0])); i++)
		{
			p_TileArray[i].MoveLeft();
		}
	}
}

void Block::MoveRight()
{
	if(Grounded == false)
	{
		for(int i = 0; i < (sizeof(p_TileArray) / sizeof(p_TileArray[0])); i++)
		{
			p_TileArray[i].MoveRight();
		}
	}
}

void Block::MoveDown()
{
	if(Grounded == false)
	{
		for(int i = 0; i < (sizeof(p_TileArray) / sizeof(p_TileArray[0])); i++)
		{
			p_TileArray[i].MoveDown();
		}
	}
}

void Block::Rotate()
{
}

void Block::Render()
{
	for(int i = 0; i < (sizeof(p_TileArray) / sizeof(p_TileArray[0])); i++)
	{
		p_TileArray[i].Render();
	}	
}

Iain Clarke, Warrior Programmer30-Sep-08 1:25

30-Sep-08 1:25

On the code above, I'm quite surprised at the delete [] failing, unless there is a problem inside the destructor for Tile.

Have you tried making a test app, and tried Tile as a very simple class (ie, just an int), and implicit constructor / destructor?

Also put a breakpoint on your allocation and deallocation part, and make sure you haven't messed up the address of p_TileArray.

Lastly, I can see one other bug, which is in place many times.

for(int i = 0; i < (sizeof(p_TileArray) / sizeof(p_TileArray[0])); i++)

As m_pTileArray is a pointer, sizeof(p_TileArray) will be 4 on a 32 bit system, and 8 on a 64 one. I'm assuming sizeof (Tile) is larger, so your loops will never work. When you weren't dynamically allocating Tiles, and had Tile p_TileArray[4]; (I'm guessing), then the sizeof trick would work nicely.

You;'ll need a separate member variable to tell the loops how many you have.

eg:

p_TileArray = NULL;
n_TileArray = 4;
p_TileArray = new Tile[n_TileArray];

You may also want to use array structures to hold the tiles, which will clear things up.

In block.h
    CArray<Tile, Tile &> m_Tiles;

In block.cpp
  constructor
   m_Tiles.SetSize (4);
   m_Tiles [0].Set....

...
void Block::Render()
{
	for(int i = 0; i < m_Tiles.GetSize (); i++)
	{
		m_Tiles[i].Render();
	}	
}

If you prefer, you could use std::vector instead. Either choice frees you from needing to delete the dynamic "array", and may help you.

Good luck,

Iain.

Sauce!30-Sep-08 1:34

30-Sep-08 1:34

thanks for pointing that out Smile | :)

I would use a vector but it seems overkill as there's only 4 elements in the array. The reason I'm dynamically allocating them is because as far as I know, it's not possible to allocate four tile's to each block before compile time. It just doesn't work.

I considered that maybe it was actually a problem with the Tile's destructor, but couldn't spot anything out of place. The following is the constructor and destructor for the Tile class;

Tile::Tile(void)
{
	*p_iNumTiles++;
	left = 0;
	top = 0;
	depth = 0;

	sizex = 0;
	sizey = 0;
	sizez = 0;
}

Tile::~Tile(void)
{
	*p_iNumTiles--;
}

Iain Clarke, Warrior Programmer30-Sep-08 1:48

30-Sep-08 1:48

Sauce! wrote:
as far as I know, it's not possible to allocate four tile's to each block before compile time

I don't see why not - unless the Tile class requires knowledge of the Block class, simply declare it earlier, then make a static array. 99% of your problems go away. You could also use a #define NUMTILESPERCLOCK 4 rather hard wiring 4, and use the define in your loops.

The bit that worries me in the code you posted is the

Sauce! wrote:

*p_iNumTiles++;
*p_iNumTiles--;

If this is a pointer to some ref counting integer, where is it assigned? If it is a global integer, why are you using pointers? Maybe it's valid during the constructor stage, but not during the destructor, in which case bad things happen - as you see...

Without knowing more of the why / how of p_iNumTiles I run out of steam. Can you get rid of it, and see if all gets better?

Iain.

Sauce!30-Sep-08 2:16

30-Sep-08 2:16

Oh dear, I am so silly :P

That ought to teach me not to reuse old code that has never been tested.

It was simply a pointer to a static int (that was supposed to be) declared in the tile class. Stupid me, put it outside of the tile class for some unknown reason, which made it inaccessible to the Tile class. D'Oh! | :doh:

Unfortunately though, removing it doesn't fix the problem Frown | :(

I will try what you said and get back to you, though for the mean time I have another question: How do you use a static variable to count the number of classes you have? I can do everything but initialise the variable - C++ simply doesn't let you initialise non const member variables within a class, so where am i supposed to initialise the count to 0? Take the following code as an example;

class theClass
{
public:
theClass(void);
~theClass(void);

private:
static int theCount = 0; //Error!
};

theClass::theClass()
{
theCount++
}

theClass::~theClass()
{
theCount--
}

Iain Clarke, Warrior Programmer30-Sep-08 3:21

30-Sep-08 3:21

Header:
class CMyClass
{
public:
    CMyClass ();
    ~CMyClass ();

protected:
   static int g_MyCounter;
};

cpp file:

int CMyClass::g_MyCounter = 0; // initialise it!

CMyClass::CMyClass ()
{
   g_MyCounter++;
}

CMyClass::~CMyClass ();
{
   ASSERT(g_MyCounter); // vulnerable to race conditions in a multithreaded app. Use crit_section if it's that, um, critical
   g_MyCounter--;
}

Given that you have a fixed number per block, I really can't see any point in you doing this for this application though... That static member variable is shared between all instances of CMyClass. I use this technique a lot for registered messages.

eg:

UINT CMyTopSecretClass::m_wm_SomeMessage = ::RegisterWindowMessage ("CMyTopSecretClass::m_wm_SomeMessage");

I hope this helps,

Iain.

Sauce!30-Sep-08 5:01

30-Sep-08 5:01

Thanks, I thought that's how you were meant to do it, but it just doesn't feel "right" because you're assigning a value to something outside of a function... so when does that value get assigned? During compile-time? At run-time?

I've done what you said and simply created an array of Tile's with 4 elements... Now i get the error

"Error 7 error C2148: total size of array must not exceed 0x7fffffff bytes c:\users\bryce\documents\visual studio 2008\projects\...\...\block.h 29"

My tile class certainly isn't that large... (half a terabyte if my calculations are correct!)

If I can't figure that out then I'll involve vectors >_>

EDIT: it may interest you to know that I also get the error "Error 2 error C2079: 'Block::TileArray' uses undefined class 'Tile' c:\users\bryce\documents\visual studio 2008\projects\...\...\block.h 29"

This is the problem I was coming across earlier and is the reason I thought it wasn't possible to do this. I have a forward declaration for the Block class right before my Tile class, but there is still no way for it to know it's definition. How can I overcome this?

modified on Tuesday, September 30, 2008 11:09 AM

Iain Clarke, Warrior Programmer30-Sep-08 5:31

30-Sep-08 5:31

Sauce! wrote:
My tile class certainly isn't that large... (half a terabyte if my calculations are correct!)

OK, I in no way suspected that the tile class was HUMUNGOUS. And I also find myself a lot skeptical about half a terabyte. That fills half of my biggest harddrive, let alone many multiples of my swap file...

If you really meant it about the Tile class ending up being half a terabyte, then your initial problem is probably dead easy:
"The initial new failed, and p_MyTiles ends up being NULL, and so the delete [] chokes on NULL. The loops weren't crashing due to the sizeof problem with dynamic arrays I mentioned earlier".

As for the Block / Tile interaction... You have to pick either Block or Tile to know about the other. You can forward declare Block, and then use pointers or references (same thing really nearasdammit) in the other.

Ie:

class Block;   // forward declare, so the compiler knows "Block" is a valid name.

class Tile
{
   ...
   void SetParentBlock (Block *pBlock);  // only needs to know that Block exists, nothing more.
   ...
};

class Block
{
   ...
   Tile m_Tiles [4]; // needs to know all about Block's size, so needs to see it's full definition.
   ...
};

I hope that clears it up a bit.

Iain.

Sauce!30-Sep-08 5:43

30-Sep-08 5:43

It should then, be as simple as putting #include "Tile.h" at the top of Block.h but this still doesn't quite do it Frown | :(

Those two errors remain.

Iain Clarke, Warrior Programmer30-Sep-08 6:02

30-Sep-08 6:02

That won't work if you also have #include "Block.h" at the top of "Tile.h" for example...

I really recommend throwing away most of the insides of those two classes until you narrow down the trouble.

Iain.

Sauce!2-Oct-08 5:05

2-Oct-08 5:05

I've been thinking hard about this and the only thing I can think of that might cause this problem is my constructors (the default constructor and a second constructor that takes two parameters.

Block::Block(void)
{
	Grounded = false;
	Shape = (rand() % 7);
	Colour = (rand() % 6);

	p_TileArray = NULL;
	p_TileArray = new Tile[4];

	p_TileArray[0].SetPos(498.0, 82.0);
	p_TileArray[1].SetPos(530.0, 82.0);
	p_TileArray[2].SetPos(498.0, 114.0);
	p_TileArray[3].SetPos(530.0, 114.0);
}

Block::Block(int itsShape, int itsColour)
{
	Grounded = false;
	Shape = itsShape;
	Colour = itsColour;

	p_TileArray = NULL;
	p_TileArray = new Tile[4];

	switch(Shape)
	{
	case SHAPE_O:
		{
			p_TileArray[0].SetPos(498.0, 82.0);
			p_TileArray[1].SetPos(530.0, 82.0);
			p_TileArray[2].SetPos(498.0, 114.0);
			p_TileArray[3].SetPos(530.0, 114.0);
			break;
		}

	case SHAPE_T:
		{
			p_TileArray[0].SetPos(530.0, 82.0);
			p_TileArray[1].SetPos(498.0, 114.0);
			p_TileArray[2].SetPos(530.0, 114.0);
			p_TileArray[3].SetPos(562.0, 114.0);
			break;
		}

	case SHAPE_L:
		{
			p_TileArray[0].SetPos(498.0, 114.0);
			p_TileArray[1].SetPos(530.0, 114.0);
			p_TileArray[2].SetPos(562.0, 114.0);
			p_TileArray[3].SetPos(562.0, 82.0);
			break;
		}

	case SHAPE_J:
		{
			p_TileArray[0].SetPos(498.0, 82.0); 
			p_TileArray[1].SetPos(498.0, 114.0);
			p_TileArray[2].SetPos(530.0, 114.0);
			p_TileArray[3].SetPos(562.0, 114.0);
			break;
		}

	case SHAPE_S:
		{
			p_TileArray[0].SetPos(466.0, 82.0);
			p_TileArray[1].SetPos(498.0, 82.0);
			p_TileArray[2].SetPos(466.0, 114.0);
			p_TileArray[3].SetPos(498.0, 82.0);
			break;
		}

	case SHAPE_Z:
		{
			p_TileArray[0].SetPos(466.0, 82.0);
			p_TileArray[1].SetPos(498.0, 82.0);
			p_TileArray[2].SetPos(466.0, 114.0);
			p_TileArray[3].SetPos(498.0, 82.0);
			break;
		}
		
	case SHAPE_I:
		{
			p_TileArray[0].SetPos(466.0, 82.0);
			p_TileArray[1].SetPos(498.0, 82.0);
			p_TileArray[2].SetPos(466.0, 114.0);
			p_TileArray[3].SetPos(498.0, 82.0);
			break;
		}

	default:
		{
			//error
			break;
		}
	}
}

Block::~Block(void)
{
	delete [] p_TileArray;
	p_TileArray = NULL;
}

AFAIK the rest of the class doesn't matter for this problem so I'll leave it out.

The line I'm concerned with is p_TileArray = new Tile[4]; in both constructors. Only one constructor will ever be called for each instance of the class, so theoretically this shouldn't be a problem, but theory is rarely an actuality :P Is there any way this could be causing me problems? I've done a bit of fiddling around with that bit hoping to fix it by trial and error but still I've got nothing to show for the time I've invested in it Frown | :(

Sauce!4-Oct-08 2:42

4-Oct-08 2:42

I've looked into the problem a bit further and the destructor for my Block object is being called from the following area of my UpdateWorld() function;

for(int i=0; i < 10; i++)<br />
{<br />
	Block theBlock;<br />
	p_CurrentBlock = &theBlock;<br />
	vector_BlockList.push_back(*p_CurrentBlock);<br />
}

Every time the for loop iterates, theBlock falls out of scope and thus it's destructor is called. I've yet to work out why that would be a problem (and I doubt it is) but it may be helpful information to know Smile | :)

EDIT: Wow, I'm very stupid indeed :P

When theBlock falls out of scope, p_CurrentBlock becomes a wild pointer. It could be pointing to anything! Blush | :O

Another edit: Problem solved! Thankyou very much for your help Iain Smile | :)

Btw you have a very cool name Cool | :cool:

Yet another edit™: I seem to have jumped the gun. Commenting out the following code fixed it, but when I made the necessary changes to it I still get the same problem! Frown | :(

for(int i=0; i < 10; i++)<br />
{<br />
Block theBlock;	<br />
vector_BlockList.push_back(theBlock);<br />
p_CurrentBlock = &vector_BlockList.back();<br />
}

Strangely, though THIS code, that is called elsewhere in my program, works fine.

if (p_CurrentBlock->isGrounded() == true)<br />
{<br />
Block theBlock;<br />
vector_BlockList.push_back(theBlock);<br />
p_CurrentBlock = &vector_BlockList.back();<br />
}

Commenting out the first code snippet makes it work, but commenting out the second one only doesn't, so it's not a matter of those two snippets conflicting with each other.

further edit:

if (p_CurrentBlock->isGrounded() == true)<br />
{<br />
Block theBlock;<br />
vector_BlockList.push_back(theBlock);<br />
p_CurrentBlock = &vector_BlockList.back();<br />
}

was always evaluating to false, and thus the statements were not executed. The culprit lies somewhere in

Block theBlock;<br />
vector_BlockList.push_back(theBlock);<br />
p_CurrentBlock = &vector_BlockList.back();

which most likely leads us back to the destructor for Tile Frown | :(

modified on Saturday, October 4, 2008 11:03 AM

Sauce!4-Oct-08 5:19

4-Oct-08 5:19

Previous post is getting long, so I'm making another.

I noticed some strange behaviour that I believe is definitely the cause of the "vector deleting destructor" error I've been getting.

I set a break point on each of the following lines and a watch on p_TileArray[0] through p_TileArray[3].

In the watch window it only shows p_TileArray for each variable, without the index. Furthermore, when I step through the breakpoints, You can see that p_TileArray is being treated as a single variable, as with every call to SetPos(), each element is given the same value. That is why the destructor is failing, as it is a vector deleting destructor and is being used as a scalar deleting destructor.

What causes this behaviour with p_TileArray and how do I fix it?"

See image here[^]

Sauce!4-Oct-08 5:34

4-Oct-08 5:34

Bloody hell this is getting annoying >_> I ought to not be posting at this time of night as I do stupid things.

Anyway, turns out it is being treated as a regular array, like it should.

It's probably worth noting that the constructor is being called twice by the code, which should not happen.

if (p_CurrentBlock->isGrounded() == true)<br />
{<br />
Block theBlock;<br />
vector_BlockList.push_back(theBlock);<br />
p_CurrentBlock = &vector_BlockList.back();<br />
}

and immediately after stepping out of the constructor for the second time, we get a crash / assert failure, and every element of p_TileArray loses its data and becomes -1.5883997e+038 >_>

EDIT: I really think I may have figured it out now - I haven't defined a copy constructor for my Block class, so when theBlock is copied into vector_BlockList and we exit the if statement, all the Tile objects created on the heap are deleted, whilst theBlock that was passed into vector_BlockList still has a pointer that refers to them.

I've yet to implement the copy constructor as it's nearly 2 AM over here, but it's first on the table for tomorrow. Here's hoping for a good day tomorrow :P

modified on Saturday, October 4, 2008 11:46 AM

Sauce!4-Oct-08 12:53

4-Oct-08 12:53

Okay so now I've built the copy constructor and I'm fairly certain it works perfectly, however now when I step through the program, The constructor for Block is being called once, followed by it's destructor, which now exhibits normal behaviour, first deleting p_TileArray and then setting it to NULL. Then the constructor is being called a second time, once again followed by the destructor, which also exhibits normal behaviour, but THEN the destructor is called a second time without another call to the constructor, which ends with a Debug assertion failure Frown | :(

EDIT:

for(int i=0; i < 10; i++)<br />
{<br />
Block theBlock;	<br />
vector_BlockList.push_back(theBlock);<br />
p_CurrentBlock = &vector_BlockList.back();<br />
}

After the second iteration of this loop, vector_BlockList contains two of my Block class. The watch window tells me that the member variables are:

vector_BlockList[0].Shape = 5
vector_BlockList[0].Colour = 2
vector_BlockList[0].Grounded = false
vector_BlockList[0].p_TileArray = 0x00d15c7c {left=-1.5883997e+038 top=-1.5883997e+038 depth=-1.5883997e+038 ...} //p_TileArray has lost it's value after being inserted into the List.

vector_BlockList[1].Shape = 1
vector_BlockList[1].Colour = 4
vector_BlockList[1].Grounded = false
vector_BlockList[1].p_TileArray = p_TileArray = 0x00d15c7c {left=-1.5883997e+038 top=-1.5883997e+038 depth=-1.5883997e+038 ...} //Contains the same memory as vector_BlockList[0].p_TileArray - why?

modified on Saturday, October 4, 2008 7:18 PM

Sauce!17-Oct-08 20:58

no WM_MOUSEHOVER in an edit

17-Oct-08 20:58

finally solved Wink | ;)

It was an error in my copy constructor for the Block and Tile classes. Because the copy constructor was const, it had to invoke const-only methods. Turns out I stuffed up the syntax and used

const int GetSomeVar()
{
return SomeVar;
}

instead of

int GetSomeVar() const
{
return SomeVar;
}

Thanks to everyone who helped.

followait28-Sep-08 18:58

followait

28-Sep-08 18:58

Even in CEdit::PreTranslateMessage I can't meet WM_MOUSEHOVER.
No problem with CListCtrl,
even without a call to TrackMouseEvent,
and I'm not sure if TrackMouseEvent is necessary any more.

logics

Re: no WM_MOUSEHOVER in an edit

Iain Clarke, Warrior Programmer30-Sep-08 0:25