Link to home
Start Free TrialLog in
Avatar of alfredj
alfredj

asked on

new fails in this program

Hi, and Thank You for considering this question.

I want to create an array of pointers to structure and allocate
the structures one-by-one in a for() loop.  This program fails
on or about the 19th iteration with an Access Violation while
executing "particle[p] = new PARTICLE"

I am using both 95 & NT4 and VC++5.

Here is the code. It is all here in one short file:

#include <windows.h>
#include <string.h>
#include <time.h>
#include <stdio.h>

typedef struct _PARTICLE_ {
   int id;
   int xPos;
   int yPos;
   int xDelta;
   int yDelta;
   int lifespan;
   int age;
   int red, green, blue;
   int oldX, oldY;
} PARTICLE;

LRESULT CALLBACK WndProc (HWND, UINT, WPARAM, LPARAM) ;
void DrawSomething (HWND hwnd);
static int cxClient, cyClient ;
static int xMouse, yMouse;

int WINAPI WinMain (HINSTANCE hInstance, HINSTANCE hPrevInstance,
                    PSTR szCmdLine, int iCmdShow)
{
   static char szAppName[] = "RandomWalk" ;
   HWND        hwnd ;
   MSG         msg ;
   WNDCLASSEX  wndclass ;

   wndclass.cbSize        = sizeof (wndclass) ;
   wndclass.style         = CS_HREDRAW | CS_VREDRAW ;
   wndclass.lpfnWndProc   = WndProc ;
   wndclass.cbClsExtra    = 0 ;
   wndclass.cbWndExtra    = 0 ;
   wndclass.hInstance     = hInstance ;
   wndclass.hIcon         = LoadIcon (NULL, IDI_APPLICATION) ;
   wndclass.hCursor       = LoadCursor (NULL, IDC_ARROW) ;
   wndclass.hbrBackground = (HBRUSH) GetStockObject (WHITE_BRUSH) ;
   wndclass.lpszMenuName  = NULL ;
   wndclass.lpszClassName = szAppName ;
   wndclass.hIconSm       = LoadIcon (NULL, IDI_APPLICATION) ;

   RegisterClassEx (&wndclass) ;

   hwnd = CreateWindow (szAppName,
                        "Random Walk",
                        WS_OVERLAPPEDWINDOW | WS_VSCROLL,
                        CW_USEDEFAULT,
                        CW_USEDEFAULT,
                        640, /* width */
                        480, /* height */
                        NULL,
                        NULL,
                        hInstance,
                        NULL) ;

   ShowWindow (hwnd, iCmdShow) ;
   UpdateWindow (hwnd) ;

   while ( TRUE ) {

      if ( PeekMessage (&msg, NULL, 0, 0, PM_REMOVE) ) {

         if ( msg.message == WM_QUIT ) {
            break;
         }

         TranslateMessage (&msg) ;
         DispatchMessage (&msg) ;

      } else {

         DrawSomething (hwnd);

      }
   }
   return msg.wParam ;
}

LRESULT CALLBACK WndProc (HWND hwnd, UINT iMsg, WPARAM wParam, LPARAM lParam)
{
   switch ( iMsg ) {
   
   case WM_SIZE:
      cxClient = LOWORD (lParam);
      cyClient = HIWORD (lParam);
      return 0;

   case WM_DESTROY:
      PostQuitMessage (0) ;
      return 0 ;

   case WM_MOUSEMOVE:
      xMouse = LOWORD(lParam);
      yMouse = HIWORD(lParam);
      return 0;
   }

   return DefWindowProc (hwnd, iMsg, wParam, lParam) ;
}

void
DrawSomething (HWND hwnd)
{
   HDC hdc;
   time_t theTime;

   const int maxParticles = 300;

   PARTICLE *particle[maxParticles];
   static int theEpoch;
   static int t;

   int  p = 0;

   if ( cxClient == 0 || cyClient == 0 ) {
      return;
   }

   /* initialize the particles */
   if ( theEpoch == 0 ) {
      theEpoch++;

      srand ((unsigned) time (&theTime));

      for ( p = 0; p < maxParticles; p++ ) {

       particle[p] = new PARTICLE;   /* Access violation here at 19th iteration */

         memset (particle[p], 0, sizeof(particle));

         particle[p]->id = p;

         particle[p]->xPos = cxClient / 2;
         particle[p]->yPos = cyClient / 2;

         particle[p]->xDelta = (rand() % 3) - 1;
         particle[p]->yDelta = (rand() % 3) - 1;

         particle[p]->lifespan = (rand() % 10) + 5;

         particle[p]->age = 0;

         particle[p]->red   = rand() % 256;
         particle[p]->green = rand() % 256;
         particle[p]->blue  = rand() % 256;

      }

      p = 0;
   }

   /* set the particle counter */
   p = t++ % maxParticles;

   /* paint the particle */
   hdc = GetDC (hwnd);

   ReleaseDC (hwnd, hdc);

}







Avatar of ecw
ecw

Try changing
    memset (particle[p], 0, sizeof(particle));
to
    memset (particle[p], 0, sizeof(PARTICLE));
memset (particle[p], 0, sizeof(particle));

should be

memset (particle[p], 0, sizeof(PARTICLE));
Opps, ecw, beat me to it.  His answer was not there when I started looking at the code.  He should get the points.

However to avoid this problem in the future, use a constructor.  That is what they are for!
Avatar of alfredj

ASKER

Okay great, that solved the problem.  I'll raise the points
to 150 and add one line to the program.

put "particle[p]->oldX = particle[p]->xPos;"
right after "p = t++ % maxParticles;"

and I get an Access Violation.
ASKER CERTIFIED SOLUTION
Avatar of nietod
nietod

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
This code has a bunch of problems.

The array
 PARTICLE *particle[maxParticles];
is local to a procedure and is not static, thus it is created and destroyed every time the procedure is called.  However, the procedure uses

  if ( theEpoch == 0 ) {
         theEpoch++;

to insure that the array is initialized only the first time (ever 2^32 times after that.)  That makes no sense.  Thus the 2nd time the procedure is called there is a new array with random data in it that does not get initialized.  That is the source of the current crash.

Another problem is that you allcoate the PARTICAL objects with new , but never delete them.

You need to take a different approach, but I'm not sure what is best as I'm not sure what your goal is.  A quick fix would be to make the array static, but then you would not know when to clean it up and you would have to redisign the code that prevents multiple initializions.  There are probably better fixes if you can explain the goal.

Where is t set? Or do you rely on C++ initializing auto vars to 0?
Avatar of alfredj

ASKER

Thanks!  I'll go with the quick fix of putting "static" in front
of the array definition.

I don't really have a goal other than learning about VC++
and experimenting with things I read on the net and in
magazines.  Thanks for the help.  I'd love to give some points
to ecw (because he beat you to the answer by 5 mins) but your
second answer was what really helped me.

BTW, is there a way to SEARCH the database of old questions
by keyword?
>> Where is t set? Or do you rely on C++ initializing auto vars to 0?

Strangely enough  that does't matter.  As far as I can tell, the code is used to randomly set items in the array so the initial value of "t" should not matter.

>>Thanks!  I'll go with the quick fix of putting "static" in front
>> of the array definition.
That leaves you with a memory leak.  I have no idea how you will fix it.  

Also you will need to redo the code that controls the initialization of the array.  Use a boolean instead of theEpoch to control the initialization.  The way it is now you increment TheEpoch each time the procedure is called.  Eventually it will rollover and become 0 again and then you will reinitialize the array again.  Use a static boolean that is set to false at first and then change it to true when the initialization is done.

Not that is not a good design!. But if that's what you want....
Avatar of alfredj

ASKER

I thought statics were always initialized to ZERO,
[K&R, 1st edition, page 37]
so in this case t is zero at the start
and theEpoch is zero at the start so the "then"
part of "if (theEpoch == 0) {" is only executed once and
will NOT rollover since 'theEpoch++' is inside the 'then'
clause. (Although I agree that I should use a boolean.)

't' is only used as the input to 't++ % maxParticles'
so it should not matter if it rolls over.

I know I have a memory leak, but since the only way out of
the 'while (TRUE) {' loop is to exit the program, it should
not be a big deal, but couldn't I use:

if (some_condition)
  for all particle[p]
    delete particle[p];

You guys are really great!  I really like this web community!
Thanks again!

>> I thought statics were always initialized to ZERO,
Yes, for the built-in types unless explicily initialized to something else.

>>     part of "if (theEpoch == 0) {" is only executed once and
>>     will NOT rollover since 'theEpoch++' is inside the 'then'
>>     clause. (Although I agree that I should use a boolean.)

Yes, my mistake.

>>'t' is only used as the input to 't++ % maxParticles'
>>  so it should not matter if it rolls over.

correct.

>> know I have a memory leak, but since the only way out of
>> the 'while (TRUE) {' loop is to exit the program, it should
>> not be a big deal,

I'm not sure what the while loop has to do with anything.  Unless your points is that the memory will always be needed while the program is running. However it is a bad practice to not delete all remaining allocated items when the program terminates.  That is a way around some of the safegaurds C++ puts in to try to help you write safer code.

>> but couldn't I use:
>>
>>     if (some_condition)
>>       for all particle[p]
>>         delete particle[p];

Yes, but that would have to be done inside of DrawSomething().   You would have to pass some sort of semaphore to DrawSomething() to indicate that it should deleted instead of draw.  Doesn't that seem a little weird?.  Better to use a global array and have seperate functions for initializing, drawing, and deleting.
Avatar of alfredj

ASKER

Wow!  Thanks!  I really like this!  I don't get to
talk in this much detail about computer programs
very often.  This is a fantastic web site!

You seem like you have used this site a lot.  Do you know if
I can I search other people's questions by keyword?  Or is
the chronological list the only way to go?

There is no search feature.  They were going to add one soon when I started here a year ago.  There are still going to add one soon.