How to create dynamic array of structs using realloc in function

Question

I'm trying to create dynamic array of structs (pics in this case) using realloc. I want to do it in function, but there's a problem: variables in main are not changed by this function. There must be problem with pointers but I cannot find it. PS: Function UploadFile works properly. There's a function:

int AddPicture(struct Picture ***tab, int *PicCounter)
{
    struct Picture *temp;
    struct Picture pic;
    (*PicCounter)++;
    temp = realloc(*tab, (*PicCounter) *sizeof(*temp));
    if (temp != NULL)
    {
        UploadFile(&pic);
        **tab = temp;
        (*tab)[*PicCounter-1]->name = pic.name;
        (*tab)[*PicCounter-1]->type[0] = pic.type[0];
        (*tab)[*PicCounter-1]->type[1] = pic.type[1];
        (*tab)[*PicCounter-1]->width = pic.width;
        (*tab)[*PicCounter-1]->height = pic.height;
        (*tab)[*PicCounter-1]->scale = pic.scale;
        (*tab)[*PicCounter-1]->table = pic.table;
    }
    else {
        printf("Memory reallocation error");
        free(temp);
        return 1;
    }
return 0;
}

There's how I call it in main:

struct Picture *tabofpics;
int piccounter = 0;
tabofpics = malloc(1 * sizeof(*tabofpics));
AddPicture(&tabofpics,&piccounter);

Thanks for help.

EDIT: I've tried **tab instead of ***tab and values in main are correct, but it acts like memory doesn't reallocate properly, even if realloc doesn't return NULL.

  int AddPicture(struct Picture **tab, int *PicCounter)
{
    struct Picture *temp;
    struct Picture pic;
    (*PicCounter)++;
    temp = realloc(*tab, (*PicCounter) * sizeof(*temp));
    if (temp != NULL)
    {
    UploadFile(&pic);
    *tab = temp;    
    tab[*PicCounter - 1]->name = pic.name;
    tab[*PicCounter - 1]->type[0] = pic.type[0];
    tab[*PicCounter - 1]->type[1] = pic.type[1];
    tab[*PicCounter - 1]->width = pic.width;
    tab[*PicCounter - 1]->height = pic.height;
    tab[*PicCounter - 1]->scale = pic.scale;
    tab[*PicCounter - 1]->table = pic.table;
}
else {
    printf("Memory reallocation error");
    free(*tab);
    return 1;
}
return 0;
}                                                                                                                             

The idea is to put as many pics in the program as one want to, do some operations on its and leave. There must be function to add and delete pics whenever user want to, however when I call function with **tab in argument second time I get Access violation location, so as I mentioned earlier, realloc must not work properly.

Answer 1

According to answers of @Iharob and @John I was able to write a working function:

 int AddPicture(struct Picture **tab, int *PicCounter)
{
struct Picture *temp;
(*PicCounter)++;
temp = realloc(*tab, (*PicCounter) * sizeof(*temp));
if (temp != NULL)
{
    struct Picture *pic;
    pic = malloc(sizeof(*pic));
    UploadFile(pic);
    *tab = temp;
    (*tab)[(*PicCounter)-1] = *pic;
    free(pic);
}
else {
    printf("Memory reallocation error");
    free(*tab);
    (*PicCounter)--;
    return 1;
}
return 0;
}

It's not perfect, but it works properly. Thanks for help!

Answer 2

The central problem can perhaps be best summarized by these two excerpts from your code:

    temp = realloc(*tab, (*PicCounter) *sizeof(*temp));

[...]

        **tab = temp;

You are reallocating the space pointed to by the value of *tab, but on success, you're storing the pointer to the new space in **tab, which is a different location. Regardless of which of those is the right location for the pointer, the combination is certainly wrong.

In fact, however, given the way you are calling your function ...

struct Picture *tabofpics;

[...]

AddPicture(&tabofpics,&piccounter);

... the type of the actual argument is struct Picture **. And that's appropriate for your purpose, so you should adjust the function definition to match:

int AddPicture(struct Picture **tab, int *PicCounter)

. Moreover, you should ensure that a matching function declaration is present in a header file that is #included into the source file containing the definition of AddPicture and into every source file containing a call to that function. That, plus ensuring that compiler warnings are turned up, should empower the compiler to help you diagnose mismatches between the definition and the uses of this function.

Having done that, the expression *tab in function AddPicture() will refer to the same object that the expression tabofpics (no *) refers to in main(). That appears to be consistent with how you're actually using it, with the exception of the mismatch I described at the beginning.

There are other reasonable criticisms of your code that could be made, but @Iharob has already done a good job of addressing those in his answer.

Answer 3

As I see it, you need to improve the logic of your code, for instance

1. Don't increment PicCounter yet
2. You have a mismatch in your pointers indirection level, in the caller function you have struct Picture * and you are passing it as struct Picture ** to a function taking struct Picture ***. You should reason about it and what is it that you really want. It seems that you want an array of struct Picture *.

Style,

1. Don't use PicCounter for a variable, use number_of_pictures or something more appropriate.
2. Don't write 1 * sizeof(...) it's too explicit and you don't need to be as much.

Tooling,

1. Enable compiler warnings to spot simple mistakes that you make without noticing, or those that are a conceptual flaw like the mismatched indirection of pointers in your code.

That said, I think you probably wanted to write this

int
AddPicture(struct Picture ***tab, int *counter)
{
    struct Picture **array;   
    array = realloc(*tab, (*counter + 1) * sizeof(*array));
    if (array != NULL)
    {
        struct Picture *current;
        // We will avoid using the `tab' variable because it's
        // confusing, the only reason why it's a *** pointer is
        // to do this here, and below where we set it to `NULL'
        *tab = array;
        // We also need to allocate space for a new image to store
        // the results in.
        current = malloc(sizeof(*current));
        if (current == NULL)
            goto error;    
        // This is a guess, but isn't the code just initializing
        // the `pic' that you allocated on the stack?
        //
        // Well, the following is the same.
        //
        // Oh, and should we check for errors here?
        UploadFile(current);            
        // Now we can increase counterand update our array
        array[(*counter)++] = current;
        // We have succeded, so return 0
        return 0;
    }
error:
    printf("Memory reallocation error");
    // Release memory allocated so far
    free(*tab);
    // Prevent dangling pointer
    *counter = 0;
    *tab = NULL;
    // A non zero value means, an error
    return 1;
}

And the caller function should be something like

struct Picture **tabofpics;
int counter = 0;
// `realloc' will take car of the allocation
tabofpics = NULL;
if (AddPicture(&tabofpics, &picounter) == 0)
    // Everything ok, picture added
else
    // Error

Please be careful with *** pointers, they can cause a lot of confusion. I have to think 4 or 5 times about

(*tab)[*counter] = current;

before I can see that it's correct. So I chose to change it as you can see in the code.

Your code should be as descriptive as possible, and using *** pointers goes in the wrong direction of that.