realloc from within a function

Question

I have a struct called ball, a number of balls, an array of balls and a function where I want to add new balls to the array:

The struct:

typedef struct ball{
    BITMAP *image;
    int x;
    int y;
    int vector_x;
    int vector_y;
} ball;

The (non working function):

void add_balls(int *num_balls, ball **myballs){
    num_balls++;
    *myballs = realloc(*myballs, *num_balls * sizeof(ball));
    *myballs[*num_balls-1]->x =  rand() % 640;
    *myballs[*num_balls-1]->y = rand() % 480;
    *myballs[*num_balls-1]->vector_x = rand() % 10;
    *myballs[*num_balls-1]->vector_y = rand() % 10;
    *myballs[*num_balls-1]->image = load_bitmap("blue_ball.bmp", NULL); 
 }

and the function call within main:

add_balls(&num_balls, &myballs);

The call to the function fails with the following error messages:

p.c: In function ‘add_balls’:
p.c:19: error: invalid type argument of ‘unary *’
p.c:20: error: invalid type argument of ‘unary *’
p.c:21: error: invalid type argument of ‘unary *’
p.c:22: error: invalid type argument of ‘unary *’
p.c:23: error: incompatible types in assignment

Any help?

That helped. It compiles now, but I get a segmentation fault error in runtime. Here is the complete code, if it interests:

#include <allegro.h>
#include <stdlib.h>
#include <time.h>
#include <math.h>

#define NUM_BALLS 10

typedef struct ball{
    BITMAP *image;
    int x;
    int y;
    int vector_x;
    int vector_y;
} ball;

void add_balls(int *num_balls, ball **myballs){
    num_balls++;
    myballs = realloc(*myballs, *num_balls * sizeof(ball));
    myballs[*num_balls-1]->x =  rand() % 640;
    myballs[*num_balls-1]->y = rand() % 480;
    myballs[*num_balls-1]->vector_x = rand() % 10;
    myballs[*num_balls-1]->vector_y = rand() % 10;
    myballs[*num_balls-1]->image = load_bitmap("blue_ball.bmp", NULL); 
 }

int main() 
{
    allegro_init();
    install_keyboard();
    srand(time(0));

    install_timer();

    set_color_depth(32);
    set_gfx_mode(GFX_AUTODETECT_WINDOWED, 640,480,0,0);

    BITMAP *buffer = NULL;
    buffer = create_bitmap(640,480);

    ball *myballs;
    myballs  = malloc(NUM_BALLS * sizeof(ball));
    int num_balls = NUM_BALLS;

    BITMAP *bg = NULL;
    bg = load_bitmap("bg.bmp",NULL);

    int i;
    for(i=0;i<num_balls;i++){
     myballs[i].x =  rand() % 640;
     myballs[i].y = rand() % 480;
     myballs[i].vector_x = rand() % 10;
     myballs[i].vector_y = rand() % 10;
     myballs[i].image = load_bitmap("blue_ball.bmp", NULL);
    }

    int bg_vector_x;
    float bg_vector_y;

    while(!key[KEY_ESC]){
     vsync();
     for(i=0;i<num_balls;i++){

      if(myballs[i].x + myballs[i].vector_x > 640 || myballs[i].x + myballs[i].vector_x < 0){
       myballs[i].vector_x *= -1;
      }
      if(myballs[i].y + myballs[i].vector_y > 480 || myballs[i].y + myballs[i].vector_y < 0){
       myballs[i].vector_y *= -1;
      }

      myballs[i].x += myballs[i].vector_x;
      myballs[i].y += myballs[i].vector_y;
     }

     if(key[KEY_UP]){
      add_balls(&num_balls, &myballs);
     }

     clear_bitmap(buffer);

     int ii;  
     for(i=0;i<3;i++){
      for(ii=-1;ii<3;ii++){
       draw_sprite(buffer, bg ,(bg_vector_x%528)+(i*528),100*cos(bg_vector_y) + ii*353);  
      }
     }
     bg_vector_x++;
     bg_vector_y+=0.1;

     for(i=0;i<num_balls;i++){
      draw_sprite(buffer, myballs[i].image, myballs[i].x,myballs[i].y);
     }

     blit(buffer, screen, 0,0,0,0,640,480);
    }

    for(i=0;i<num_balls;i++){
     destroy_bitmap(myballs[i].image);
    }
    free(myballs);
    destroy_bitmap(buffer);
}

END_OF_MAIN()

Answer 1

*myballs[*num_balls-1]->x

This dereferences one time too many - myballs is an array of pointers (or put another way, a pointer to the first element of an array of pointers) - just lose the asterisk in front of myballs, and now you'll get the pointer from which you can access the struct (or alternatively, turn the -> into a .). The symbol -> is shorthand for you to access the members of the struct from the pointer to that struct, not from the dereferenced pointer to the struct.

myballs[*num_balls-1]->x

or

(*myballs[*num_balls-1]).x

Answer 2

In your modified add_balls function you increase the pointer num_balls, you actually want to increase the content of the pointer, (*num_balls)++;

Answer 3

In addition to Adam Rosenfield's and Baruch Even's answers, you should consider what happens when realloc() fails.

p = realloc(p, s) is dangerous because when it fails it sets the pointer to NULL but doesn't dealloc the memory that it used to point to. If this happens, your program will end up leaking the original block of memory (if it doesn't crash first). Instead, you should assign the result of realloc() to another variable so that you can handle the case where it returns NULL.

Answer 4

Assuming I understand what you are trying to do, something like the following (untested) is better:

#include <assert.h>

void add_balls(int *num_balls, ball **myballs){
    int n = ++*num_balls;
    ball *pb = realloc(*myballs, n * sizeof(ball));

    /* fail loudly and early on lack of memory. */
    assert(pb); 
    *myballs = pb;

    /* note the pre-decrement of n here to make obvious the common n-1 */
    pb[--n]->x =  rand() % 640;
    pb[n]->y = rand() % 480;
    pb[n]->vector_x = rand() % 10;
    pb[n]->vector_y = rand() % 10;
    pb[n]->image = load_bitmap("blue_ball.bmp", NULL); 
 }

Although it would be possible to get all the operator precedence correct with enough parentheses in the array element references as originally written, the result will be essentially unreadable by humans. As it was, the right-to-left associativity of -> is mixing oddly with the left-to-right associativity of * and further confused by the array index.

You might want to refine the error handling. In particular, it might make sense to simply refuse to add the ball at all if memory wasn't available. In that case, it would be better to not store back the new pointer (or the new count) until it realloc() is known to have worked.

Answer 5

Your original code was very close to being correct. The first problem is relatively simple to fix. The line

num_balls++;

should be

(*num_balls)++;

You were incrementing the location to which num_balls pointed, instead of incrementing the value that it pointed to. When you later dereferenced it, it was pointing to some other piece of memory -- in this particular usage scenario, it was probably pointing to the next local variable on the stack in main(), the variable bg.

Your other problem is an order of operations problem. Array indexing has higher operator precedence than dereferencing, so the line

*myballs[*num_balls-1]->x = rand() % 640;

is equivalent to

*(myballs[*num_balls-1]->x) = rand() % 640

myballs is pointer to a pointer to an array of balls. If *num_balls-1 is anything other than 0, you will be accessing the wrong memory. Regardless, the member variable x is not a pointer, so you can't dereference it, so the compiler saves you here with a syntax error. The correct code is to make sure you dereference before taking the array index:

(*myballs)[*num_balls-1].x = rand() % 640;

This does what you want, namely dereferencing the pointer to get a pointer to the original array, taking the element you want, and assigning to the member variable x. The rest of the lines in add_ball() should be rewritten analogously.

One other issue with your code, which is more of an issue with performance than with correctness, is that you're reloading the blue_ball.bmp image for every ball. It would be better to instance the bitmap by only loading it once, and then have every ball refer to that one instance instead of reloading it each time.