crashes after displaying 100 bars??? help please.

I have written a MFC dll to interact with an API and it is run by the target program. the first 100 bars are drawn properly but then the program ceases to operate. I am guessing I have a memory leak but I probably just dont understand what I am doing as I am quite new to programming MFC GDI. Any help is greatly appreciated. Here is my code that causes the trouble...

The variables coming in are commented out to facilitate testing and so it would be displaying the same bars over and over because of this. it worrks for 100 bars, then crashes.


void MainDlg::SS_UpdateChart()
{
// the chart plot is completely recalculated each time a new bar is shown
// 300 is plottable height
// 400 is plottable width, 39 bars
int displayChartHeight = 300;

// variables used to display the chart
double displayPriceRangeHigh = 0;
double displayPriceRangeLow = 10000;
double pixelsPerDollar = 0;





CDC * myDC; // the picture box context

CPen * oldPen;

CBrush * oldBrush;


// set up the picture box
myDC = GetDlgItem(IDC_STATICPIC1)->GetDC(); // get the item context

oldPen = (CPen *) myDC->SelectStockObject(WHITE_PEN); // initialize
myDC->SelectObject(oldPen);

oldBrush = (CBrush *) myDC->SelectStockObject(WHITE_BRUSH); // initialize
myDC->SelectObject(oldBrush);




// the data for each bar is loaded into the chart array
// for each movement of the bars back the range is recalculated
// and the data is reformulated and converted to fill the chart bars

// the current bar in is bar 40 to stage it
chartBar[40].open = 15;//currentBarOpen;
chartBar[40].high = 18;//currentBarHigh;
chartBar[40].low = 12;//currentBarLow;
chartBar[40].close = 13;//currentBarClose;
chartBar[40].volume = 0;//currentBarVolume;
chartBar[40].pressure = 0;//currentBarAveragePressure;



// the displayed range of prices
// the range of chart is determined by the highest and lowest of all the bars
// being displayed at the time from second bar to the current bar being built
// only include the range of the bars that exist when first starting the display

// get the range on bars 2 to 40

for( int a = 2; a <= 40; a = a + 1)
{
if ((chartBar[a].high != 0) && (chartBar[a].high != NULL))
{
if (displayPriceRangeHigh < chartBar[a].high)
{
displayPriceRangeHigh = chartBar[a].high;
}
}

if ((chartBar[a].low != 0) && (chartBar[a].low != NULL))
{
if (displayPriceRangeLow > chartBar[a].low)
{
displayPriceRangeLow = chartBar[a].low;
}
}
}




// to avoid division by zero or null
if ((displayPriceRangeHigh < 1) || (displayPriceRangeHigh == NULL))
{
displayPriceRangeHigh = 1;
displayPriceRangeLow = 1;
}
if ((displayPriceRangeLow < 1) || (displayPriceRangeLow == NULL))
{
displayPriceRangeHigh = 1;
displayPriceRangeLow = 1;
}



// get pixels per dollar
pixelsPerDollar = displayChartHeight / (displayPriceRangeHigh - displayPriceRangeLow);



// move bar data back one column and move current bar from 40 into bar 39
// this makes bars 1 to 39 current for displaying

for( int a = 2; a <= 40; a = a + 1)
{
chartBar[(a - 1)].open = chartBar[a].open;
chartBar[(a - 1)].high = chartBar[a].high;
chartBar[(a - 1)].low = chartBar[a].low;
chartBar[(a - 1)].close = chartBar[a].close;
chartBar[(a - 1)].volume = chartBar[a].volume;
chartBar[(a - 1)].pressure = chartBar[a].pressure;
}



// clear the display for the new bars
CBrush* blackBrush = new CBrush;

blackBrush->CreateSolidBrush(blackColor);

//oldBrush = myDC->SelectObject(blackBrush);
myDC->SelectObject(blackBrush);

myDC->FillRect(new CRect (0, 0, 460, 330), blackBrush);

DeleteObject(blackBrush);

myDC->SelectObject(oldBrush);


// find colors to display for each bar space 1 column at a time
// divide the 100% into the increments of the display

int barPosition;
int barOpen;
int barHigh;
int barLow;
int barClose;


for( int a = 1; a <= 39; a = a + 1)
{

barPosition = a;

barOpen = ((chartBar[a].open - displayPriceRangeLow) * pixelsPerDollar);
barHigh = ((chartBar[a].high - displayPriceRangeLow) * pixelsPerDollar);
barLow = ((chartBar[a].low - displayPriceRangeLow) * pixelsPerDollar);
barClose = ((chartBar[a].close - displayPriceRangeLow) * pixelsPerDollar);

if (chartBar[a].open > 1)
{


COLORREF myColor;
int xSpacing = 10;
int xWidth = 5;
int xMiddle = 2;
int chartWidth = 400; // 60 on right side for moving close price, 39 bars displayed at once
int chartHeight = 300; // 15 on top and 15 on bottom
int chartBottom = 315; // 315, 15 is the top
int bodyTop; // top of the body
int bodyBottom; // bottom of the body


// items are the amounts up from the bottom
// get correct color for bar and get body top and bottom
if (barOpen < barClose) // up bar
{
myColor = greenColor;

bodyBottom = barOpen;
bodyTop = barClose;
}

if (barOpen > barClose) // down bar
{
myColor = redColor;

bodyBottom = barClose;
bodyTop = barOpen;
}

if (barOpen == barClose) // same price bar
{
myColor = yellowColor;

bodyBottom = barClose;
bodyTop = barClose;
}










// chart veiwing area is 200 high by 400 long
// show bars as 5 wide with 5 space inbetween them


CBrush* mySolidBrush = new CBrush();
mySolidBrush->CreateSolidBrush(myColor);
//oldBrush = myDC->SelectObject(mySolidBrush);
myDC->SelectObject(mySolidBrush);

// body
myDC->FillRect(new CRect ((barPosition * xSpacing),(chartBottom - barOpen),((barPosition * xSpacing) + xWidth),(chartBottom - barClose)), mySolidBrush);

DeleteObject(mySolidBrush);

myDC->SelectObject(oldBrush);


CPen* myThinPen = new CPen();//(PS_SOLID(1, myColor));
myThinPen->CreatePen(PS_SOLID,1, myColor);
//oldPen = myDC->SelectObject(&myThinPen);
myDC->SelectObject(myThinPen);

// upper shadow
myDC->MoveTo(((barPosition * xSpacing) + xMiddle), (chartBottom - barHigh));
myDC->LineTo(((barPosition * xSpacing) + xMiddle), (chartBottom - bodyTop));

// lower shadow
myDC->MoveTo(((barPosition * xSpacing) + xMiddle), (chartBottom - barLow));
myDC->LineTo(((barPosition * xSpacing) + xMiddle), (chartBottom - bodyBottom));

DeleteObject(myThinPen);

myDC->SelectObject(oldPen);



}

}

DeleteObject(myDC);

DeleteObject(oldBrush);

DeleteObject(oldPen);

//ReleaseDC(myDC);



dayBarCount = dayBarCount + 1; // bars built so far this session

} // end void MainDlg::SS_UpdateChart()

No idea from looking at the code. What does your debugger say?

A couple of things though.

1) please learn to use code tags

2)

Code:

---

if ((displayPriceRangeLow < 1) || (displayPriceRangeLow == NULL))
{
displayPriceRangeHigh = 1;
displayPriceRangeLow = 1;
}
// get pixels per dollar
pixelsPerDollar = displayChartHeight / (displayPriceRangeHigh - displayPriceRangeLow);

---

You have potential division by zero there.

Also, typically NULL is used to test pointer values, not POD types. And since your first test is < 1, NULL would also be < 1, so even if testing for NULL was valid, it would be redundant.

Thanks, I totally missed that division by zero issue there. When testing I use a constant and know that that is not the issue however. I will try this repost with the code tags, great suggestion. The code for drawing the bars or clearing the screen seems to be my trouble - I am not sure that I am getting the resources deleted properly or even setting them up properly. This part:


[code c++]

// clear the display for the new bars
CBrush* blackBrush = new CBrush;

blackBrush->CreateSolidBrush(blackColor);

//oldBrush = myDC->SelectObject(blackBrush);
myDC->SelectObject(blackBrush);

myDC->FillRect(new CRect (0, 0, 460, 330), blackBrush);

DeleteObject(blackBrush);

myDC->SelectObject(oldBrush);


// find colors to display for each bar space 1 column at a time
// divide the 100% into the increments of the display

int barPosition;
int barOpen;
int barHigh;
int barLow;
int barClose;


for( int a = 1; a <= 39; a = a + 1)
{

barPosition = a;

barOpen = ((chartBar[a].open - displayPriceRangeLow) * pixelsPerDollar);
barHigh = ((chartBar[a].high - displayPriceRangeLow) * pixelsPerDollar);
barLow = ((chartBar[a].low - displayPriceRangeLow) * pixelsPerDollar);
barClose = ((chartBar[a].close - displayPriceRangeLow) * pixelsPerDollar);

if (chartBar[a].open > 1)
{


COLORREF myColor;
int xSpacing = 10;
int xWidth = 5;
int xMiddle = 2;
int chartWidth = 400; // 60 on right side for moving close price, 39 bars displayed at once
int chartHeight = 300; // 15 on top and 15 on bottom
int chartBottom = 315; // 315, 15 is the top
int bodyTop; // top of the body
int bodyBottom; // bottom of the body


// items are the amounts up from the bottom
// get correct color for bar and get body top and bottom
if (barOpen < barClose) // up bar
{
myColor = greenColor;

bodyBottom = barOpen;
bodyTop = barClose;
}

if (barOpen > barClose) // down bar
{
myColor = redColor;

bodyBottom = barClose;
bodyTop = barOpen;
}

if (barOpen == barClose) // same price bar
{
myColor = yellowColor;

bodyBottom = barClose;
bodyTop = barClose;
}










// chart veiwing area is 200 high by 400 long
// show bars as 5 wide with 5 space inbetween them


CBrush* mySolidBrush = new CBrush();
mySolidBrush->CreateSolidBrush(myColor);
//oldBrush = myDC->SelectObject(mySolidBrush);
myDC->SelectObject(mySolidBrush);

// body
myDC->FillRect(new CRect ((barPosition * xSpacing),(chartBottom - barOpen),((barPosition * xSpacing) + xWidth),(chartBottom - barClose)), mySolidBrush);

DeleteObject(mySolidBrush);

myDC->SelectObject(oldBrush);


CPen* myThinPen = new CPen();//(PS_SOLID(1, myColor));
myThinPen->CreatePen(PS_SOLID,1, myColor);
//oldPen = myDC->SelectObject(&myThinPen);
myDC->SelectObject(myThinPen);

// upper shadow
myDC->MoveTo(((barPosition * xSpacing) + xMiddle), (chartBottom - barHigh));
myDC->LineTo(((barPosition * xSpacing) + xMiddle), (chartBottom - bodyTop));

// lower shadow
myDC->MoveTo(((barPosition * xSpacing) + xMiddle), (chartBottom - barLow));
myDC->LineTo(((barPosition * xSpacing) + xMiddle), (chartBottom - bodyBottom));

DeleteObject(myThinPen);

myDC->SelectObject(oldPen);



}

}

DeleteObject(myDC);

DeleteObject(oldBrush);

DeleteObject(oldPen);

//ReleaseDC(myDC);



dayBarCount = dayBarCount + 1; // bars built so far this session

} // end void MainDlg::SS_UpdateChart()

[/code]

Quote:

---

Originally Posted by numbertea

Thanks, I totally missed that division by zero issue there. When testing I use a constant and know that that is not the issue however. I will try this repost with the code tags, great suggestion.

---

Code tags are done this way:

[code]
Your code goes here:
[/code]


Regards,

Paul McKenzie

Code:

---

for( int a = 1; a <= 39; a = a + 1)
//...
barOpen = ((chartBar[a].open - displayPriceRangeLow) * pixelsPerDollar);

---

Arrays start at 0 in C++, not 1. Maybe it's your way of accessing an array is what is causing the problem.

What are the dimensions of your array?

Regards,

Paul McKenzie

Yes, I realize the arrays start at 0. For ease of visualizing the pixels myself I declare the arrays with one extra index so that I can start with index 1 instead of zero. Am I correct in this call?

Code:

---

// clear the display for the new bars
 CBrush* blackBrush = new CBrush;

 blackBrush->CreateSolidBrush(blackColor);

 //oldBrush = myDC->SelectObject(blackBrush);
 myDC->SelectObject(blackBrush);

 myDC->FillRect(new CRect (0, 0, 460, 330), blackBrush);

 DeleteObject(blackBrush);

 myDC->SelectObject(oldBrush);

---

Or am I missing something obvious that could be causing a memory leak. Thanks for your reply.

Quote:

---

Originally Posted by numbertea

Yes, I realize the arrays start at 0. For ease of visualizing the pixels myself I declare the arrays with one extra index so that I can start with index 1 instead of zero. Am I correct in this call?

---

This "technique" almost always leads to an off-by-one error somewhere in the program, causing the program to either crash, invalid data being used, or some other mistake due to accessing an invalid element in the array. I've even seen inconsistent usage of 0 being the start of the data in the array, and the same program somewhere else assuming that 1 is the start of the data.

Bottom line is this -- arrays start at 0, and writing "unencapsulated" code that tries to fool C++ into thinking that arrays are 1-based is a prime reason for unexpected behaviour to occur. Unless you write a well-tested C++ class that does 1-based indexing, then your loop will always be suspicious.

You say your program crashes -- that means that any part of the program could have caused this, and not just the code you are showing. How about showing us your array declaration (which I asked for but you did not provide)?

Regards,

Paul McKenzie

Quote:

---

Originally Posted by numbertea

Code:

---

// clear the display for the new bars
 CBrush* blackBrush = new CBrush;
 ...

---

Or am I missing something obvious that could be causing a memory leak. Thanks for your reply.

---

Well, where do you delete the CBrush* blackBrush object?

Quote:

---

Originally Posted by numbertea

Code:

---

// clear the display for the new bars
 CBrush* blackBrush = new CBrush;

 blackBrush->CreateSolidBrush(blackColor);

 //oldBrush = myDC->SelectObject(blackBrush);
 myDC->SelectObject(blackBrush);

 myDC->FillRect(new CRect (0, 0, 460, 330), blackBrush);

 DeleteObject(blackBrush);

 myDC->SelectObject(oldBrush);

---

---

First, as I recall, you cannot delete the object (blackbrush) while it is selected into a DC. You need to reverse the order of the last two lines.

Also, as mentioned earlier, I don't see where you delete the CBrush (blackbrush), so you have a memory leak.

Hope this helps...good luck.

Thanks for your help. I cleaned things up a little and decided to use stock objects and just change the colors. This way a memory leak is not even possible. Realizing it is a bad programming fopa, I kept my arrays for graphics starting at index '1'. I access the array many times in the program and so it just makes things simpler and lowers execution cycles necessary per loop.

Cheers!

Quote:

---

Originally Posted by numbertea

Thanks for your help. I cleaned things up a little and decided to use stock objects and just change the colors. This way a memory leak is not even possible. Realizing it is a bad programming fopa, I kept my arrays for graphics starting at index '1'. I access the array many times in the program and so it just makes things simpler and lowers execution cycles necessary per loop.

Cheers!

---

That's a horrible idea and doesn't make things simpler and I don't see how it lowers execution cycles. It'll be a maintenance nightmare. Get out of that habit immediately.

Quote:

---

Originally Posted by numbertea

I kept my arrays for graphics starting at index '1'. I access the array many times in the program and so it just makes things simpler and lowers execution cycles necessary per loop.

---

????

If you loop x times, you loop x times. It doesn't matter what the lower and upper bounds are if (upper_bound - lower_bound + 1) == x. For example, there is no difference in the number of loops done if you go from 0 to 9 or from 1 to 10. The loop is performed 10 times.

What I'm saying is that it is dangerous to use a lower bound that is not 0 for array, since arrays start at 0. Going off the end of the array, or erroneously accessing element 0 is one of the most hard to detect bugs, since many times you never know you've gone over the end until you need to run that program somewhere else (usually at a customer site, where your reputation is on the line). You either get a crash, or in the case of accessing element 0, you get bad or uninitialized data being used.

So it doesn't matter if you ran the program thousands of times on your machine seemingly without problems -- any code where you fool C++ into thinking that arrays start at 1 instead of 0 is always suspicious, especially if you're experiencing crashes. The only way to remove that doubt is to either treat your arrays correctly (start from 0), or write a class that encapsulates handling 1-based arrays, and use that class.

I have seen tons of code where the coder is doing something similar to your code with the 1-based arrays, and in almost all of the times I've seen the code, there is a bug somewhere in the accessing of the arrays. If not a bug, the code becomes a maintenance headache for anyone attempting to change the code, or even the original coder loses track and introduces bugs.

Also, you fix a crash by actually fixing the crash. By just changing code and seeing that the crash doesn't happen anymore is not a fix. All you did was rearrange the code without addressing the real problem, and that does not guarantee you fixed anything.

You need to identify why the crash occurs, and then create a fix that addresses the "why".

Regards,

Paul McKenzie