Solved

new fails in this program

Posted on 1998-08-07
13
202 Views
Last Modified: 2010-05-18
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);

}







0
Comment
Question by:alfredj
  • 7
  • 4
  • 2
13 Comments
 
LVL 5

Expert Comment

by:ecw
ID: 1169662
Try changing
    memset (particle[p], 0, sizeof(particle));
to
    memset (particle[p], 0, sizeof(PARTICLE));
0
 
LVL 22

Expert Comment

by:nietod
ID: 1169663
memset (particle[p], 0, sizeof(particle));

should be

memset (particle[p], 0, sizeof(PARTICLE));
0
 
LVL 22

Expert Comment

by:nietod
ID: 1169664
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!
0
 

Author Comment

by:alfredj
ID: 1169665
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.
0
 
LVL 22

Accepted Solution

by:
nietod earned 150 total points
ID: 1169666
answer coming.
0
 
LVL 22

Expert Comment

by:nietod
ID: 1169667
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.

0
6 Surprising Benefits of Threat Intelligence

All sorts of threat intelligence is available on the web. Intelligence you can learn from, and use to anticipate and prepare for future attacks.

 
LVL 5

Expert Comment

by:ecw
ID: 1169668
Where is t set? Or do you rely on C++ initializing auto vars to 0?
0
 

Author Comment

by:alfredj
ID: 1169669
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?
0
 
LVL 22

Expert Comment

by:nietod
ID: 1169670
>> 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....
0
 

Author Comment

by:alfredj
ID: 1169671
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!

0
 
LVL 22

Expert Comment

by:nietod
ID: 1169672
>> 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.
0
 

Author Comment

by:alfredj
ID: 1169673
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?

0
 
LVL 22

Expert Comment

by:nietod
ID: 1169674
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.
0

Featured Post

Do You Know the 4 Main Threat Actor Types?

Do you know the main threat actor types? Most attackers fall into one of four categories, each with their own favored tactics, techniques, and procedures.

Join & Write a Comment

Written by John Humphreys C++ Threading and the POSIX Library This article will cover the basic information that you need to know in order to make use of the POSIX threading library available for C and C++ on UNIX and most Linux systems.   [s…
This article will show you some of the more useful Standard Template Library (STL) algorithms through the use of working examples.  You will learn about how these algorithms fit into the STL architecture, how they work with STL containers, and why t…
The goal of the tutorial is to teach the user how to use functions in C++. The video will cover how to define functions, how to call functions and how to create functions prototypes. Microsoft Visual C++ 2010 Express will be used as a text editor an…
The viewer will be introduced to the member functions push_back and pop_back of the vector class. The video will teach the difference between the two as well as how to use each one along with its functionality.

760 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

23 Experts available now in Live!

Get 1:1 Help Now