7

I know how to build Dynamically allocated arrays, but not how to grow them.

for example I have the following interface..

void insertVertex( vertex p1, vertex out[], int *size);

This method takes a vertex and stores it into the out array. After storing the vertex I increase the count of length for future calls.

p1 - is the vertex I'm going to add.

out[] - is the array I need to store it in (which is always full)

length - the current length

Vertex is defined as..

typedef struct Vertex{
int x;
int y;
} Vertex;

This is what I'm using in Java..

Vertex tempOut = new Vertex[size +1];
//Code to deep copy each object over
tempOut[size] = p1;
out = tempOut;

This is what I believed I could use in c..

out = realloc(out, (*size + 1) * sizeof(Vertex));
out[(*size)] = p1;

However, I keep on receiving an error message that the object was not allocated dynamically.

I found a solution that will resolve this.. Instead of using Vertex* I was going to switch to Vertex** and store pointers vs. vertex. However, after switching everything over I found out that I over looked the fact that the unit test will be providing me a Vertex out[] that everything has to be stored in.

I have also tried the following with no luck.

Vertex* temp = (Vertex *)malloc((*size + 1) * sizeof(Vertex));
for(int i = 0; i < (*size); i++)
{
temp[i] = out[i];
}
out = temp;

However, no matter what I do when I test after both of these the array returned has not changed.

Update - Requested information

out - is defined as an array of Vertex (Vertex out[])

It is originally built with the number of vertex in my polygon. For example.

out = (Vertex *)malloc(vertexInPolygon * sizeof(Vertex))

Where vertexInPolygon is an integer of the number of vertex in the polygon.

length was a typo that should have been size.

Size is an integer pointer

int *size = 0;

Each time a vertex is in the clipping plane we add it to the array of vertex and increase the size by one.

Update

To better explain myself I came up with a short program to show what I'm trying to do.

#include <stdio.h>
#include <stdlib.h>

typedef struct Vertex {
    int x, y;
} Vertex;

void addPointerToArray(Vertex v1, Vertex out[], int *size);

void addPointerToArray(Vertex v1, Vertex out[], int *size)
{
    int newSize = *size;
    newSize++;
    
    out = realloc(out, newSize * sizeof(Vertex));
    out[(*size)] = v1;
    
    //  Update Size
    *size = newSize;
}

int main (int argc, const char * argv[])
{
    //  This would normally be provided by the polygon
    int *size = malloc(sizeof(int)); *size = 3;
    
    //  Build and add initial vertex
    Vertex *out = (Vertex *)malloc((*size) * sizeof(Vertex));
    Vertex v1; v1.x = 1; v1.y =1;
    Vertex v2; v2.x = 2; v2.y =2;
    Vertex v3; v3.x = 3; v3.y =3;
    
    out[0] = v1;
    out[1] = v2;
    out[2] = v3;
    
    //  Add vertex
    //  This should add the vertex to the last position of out
    //  Should also increase the size by 1;
    Vertex vertexToAdd; vertexToAdd.x = 9; vertexToAdd.y = 9;
    addPointerToArray(vertexToAdd, out, size);
    
    for(int i =0; i < (*size); i++)
    {
        printf("Vertx: (%i, %i) Location: %i\n", out[i].x, out[i].y, i);
    }
   
}
9
  • What is out defined as? What are length and size? Commented Sep 27, 2011 at 1:43
  • 4
    You said realloc complained that it wasn’t allocated dynamically. So... was it? You have to use malloc first in order to be able to use realloc later. Commented Sep 27, 2011 at 1:44
  • Can you also show the definition of length or size before it's passed to insertVertex() Commented Sep 27, 2011 at 1:45
  • evgeny out is defined as... Vertex *out = (Vertex *)malloc(sizeof(Vertex) * numOfVerInPolygon) Where numOfVerInPolygon is the number of vertex in the polygon. Daniel Brockman Yes, it is defined with malloc. Before it is passed it is defined as the following. Vertex *out = (Vertex *)malloc(sizeof(Vertex) * numOfVerInPolygon) TimothyJones Sorry, length and size are the same thing. I updated the post so that everything uses size. size starts out as zero: int *size = 0; As we go through each vertex if its in the clipping plain the size is increase by one after added to the out[](Vertex) Commented Sep 27, 2011 at 2:04
  • int *size = 0 would initialize size with NULL, and point it at nothing -- not give you a pointer to an int set to 0. Unless you're pointing it at an actual int later, this may be your problem. Commented Sep 27, 2011 at 2:18

4 Answers 4

5

One long-term problem is that you are not returning the updated array pointer from the addPointerToArray() function:

void addPointerToArray(Vertex v1, Vertex out[], int *size)
{
    int newSize = *size;
    newSize++;

    out = realloc(out, newSize * sizeof(Vertex));
    out[(*size)] = v1;

    //  Update Size
    *size = newSize;
}

When you reallocate space, it can move to a new location, so the return value from realloc() need not be the same as the input pointer. This might work while there is no other memory allocation going on while you add to the array because realloc() will extend an existing allocation while there is room to do so, but it will fail horribly once you start allocating other data while reading the vertices. There are a couple of ways to fix this:

Vertex *addPointerToArray(Vertex v1, Vertex out[], int *size)
{
    int newSize = *size;
    newSize++;

    out = realloc(out, newSize * sizeof(Vertex));
    out[(*size)] = v1;

    //  Update Size
    *size = newSize;
    return out;
}

and invocation:

out = addPointerToArray(vertexToAdd, out, size);

Alternatively, you can pass in a pointer to the array:

void addPointerToArray(Vertex v1, Vertex **out, int *size)
{
    int newSize = *size;
    newSize++;

    *out = realloc(*out, newSize * sizeof(Vertex));
    (*out)[(*size)] = v1;

    //  Update Size
    *size = newSize;
}

and invocation:

out = addPointerToArray(vertexToAdd, &out, size);

Neither of these rewrites addresses the subtle memory leak. The trouble is, if you overwrite the value you pass into realloc() with the return value but realloc() fails, you lose the pointer to the (still) allocated array - leaking memory. When you use realloc(), use an idiom like:

Vertex *new_space = realloc(out, newSize * sizeof(Vertex));
if (new_space != 0)
    out = new_space;
else
    ...deal with error...but out has not been destroyed!...

Note that using realloc() to add one new item at a time leads to (can lead to) quadratic behaviour. You would be better off allocating a big chunk of memory - for example, doubling the space allocated:

int newSize = *size * 2;

If you are worried about over-allocation, at the end of the reading loop, you can use realloc() to shrink the allocated space to the exact size of the array. However, there is then a bit more book-keeping to do; you need to values: the number of vertices allocated to the array, and the number of vertices actually in use.

Finally, for now at least, note that you should really be ruthlessly consistent and use addPointerToArray() to add the first three entries to the array. I'd probably use something similar to this (untested) code:

struct VertexList
{
    size_t    num_alloc;
    size_t    num_inuse;
    Vertex   *list;
};

void initVertexList(VertexList *array)
{
    // C99: *array = (VertexList){ 0, 0, 0 };
    // Verbose C99: *array = (VertexList){ .num_inuse = 0, .num_alloc = 0, .list = 0 };
    array->num_inuse = 0;
    array->num_alloc = 0;
    array->list      = 0;
}

void addPointerToArray(Vertex v1, VertexList *array)
{
    if (array->num_inuse >= array->num_alloc)
    {
        assert(array->num_inuse == array->num_alloc);
        size_t new_size = (array->num_alloc + 2) * 2;
        Vertex *new_list = realloc(array->list, new_size * sizeof(Vertex));
        if (new_list == 0)
            ...deal with out of memory condition...
        array->num_alloc = new_size;
        array->list      = new_list;
    }
    array->list[array->num_inuse++] = v1;
}

This uses the counter-intuitive property of realloc() that it will do a malloc() if the pointer passed in is null. You can instead do a check on array->list == 0 and use malloc() then and realloc() otherwise.

You might notice that this structure simplifies the calling code too; you no longer have to deal with the separate int *size; in the main program (and its memory allocation); the size is effectively bundled into the VertexList structure as num_inuse. The main program might now start:

int main(void)
{
    VertexList array;
    initVertexList(&array);
    addPointerToArray((Vertex){ 1, 1 }, &array);  // C99 compound literal
    addPointerToArray((Vertex){ 2, 2 }, &array);
    addPointerToArray((Vertex){ 3, 3 }, &array);
    addPointerToArray((Vertex){ 9, 9 }, &array);

    for (int i = 0; i < array->num_inuse; i++)
        printf("Vertex %d: (%d, %d)\n", i, array->list[i].x, array->list[i].y, i);

    return 0;
}

(It is coincidental that this sequence will only invoke the memory allocation once because the new size (old_size + 2) * 2 allocates 4 elements to the array the first time. It is easy to exercise the reallocation by adding a new point, or by refining the formula to (old_size + 1) * 2, or ...

If you plan to recover from memory allocation failure (rather than just exiting if it happens), then you should modify addPointerToArray() to return a status (successful, not successful).

Also, the function name should probably be addPointToArray() or addVertexToArray() or even addVertexToList().

Sign up to request clarification or add additional context in comments.

2 Comments

Leffier Thank you. Both you and Santoj really helped me out. I have updated my method and wanted to know if this would be ok. 'void addPointerToPointerArray(Vertex v1, Vertex **out, int *size) { int newSize = *size; newSize++; Vertex *tempOut = (Vertex *)malloc(sizeof(Vertex) * newSize); if (tempOut != 0) { int i = 0; while (i < *size) { tempOut[i] = (*out)[i]; i++; } free(*out); *out = tempOut; } (*out)[(*size)] = v1; *size = newSize; }'
Yes - with caveats. The while loop should probably be a for loop, or replaced with a memmove() or memcpy(). Using realloc() means you would not do the copy yourself, and it might not need to copy everything. Incrementing the size by one each time becomes a problem as you use the function more, especially in this 'always copy' mode. The first time you add one, you copy 3 items; the next, you copy 4; the next, you copy 5, etc. But what you have looks like it should do the job. (I reformatted it to eyeball it; I did not run it past a compiler, much less valgrind.)
1

I have a few suggestions for your consideration:
1. Don't use the same input & output parameter while using realloc as it can return NULL in case memory allocation fails & the memory pointed previously is leaked. realloc may return new block of memory (Thanks to @Jonathan Leffler for pointing out, I had missed this out). You could change your code to something on these lines:

Vertex * new_out = realloc(out,  newSize * sizeof(Vertex));
if( NULL != new_out )
{    
    out = new_out;
    out[(*size)] = v1;
}
else
{
 //Error handling & freeing memory
}

2. Add NULL checks for malloc calls & handle errors when memory fails.
3. Calls to free are missing.
4. Change the return type of addPointerToArray() from void to bool to indicate if the addition is successful. In case of realloc failure you can return failure say, false else you can return success say, true.
Other observations related to excessive copies etc, are already pointed out by @MatthewD.
And few good observations by @Jonathan Leffler (: Hope this helps!

2 Comments

+0.5: you've made some good points - notably about the misuse of out = realloc(out, newSize * sizeof(Vertex));. However, you've not mentioned the crucial point that realloc() might return a different pointer than the one supplied - the data might move. That fact demands a different strategy; see my answer.
@Jonathan Leffler: Thanks a lot! I had missed it out ... response edited.
0

Your sample program works fine for me. I'm using gcc 4.1.1 on Linux.

However, if your actual program is anything like your sample program, it is rather inefficient!

For example, your program copies memory a lot: structure copies - initialising out, passing vertices to addPointerToArray(), memory copies via realloc().

Pass structures via a pointer rather than by copy.

If you need to increase the size of your list type a lot, you might be better off using a linked list, a tree, or some other structure (depending on what sort of access you require later).

If you simply have to have a vector type, a standard method of implementing dynamically-sized vectors is to allocate a block of memory (say, room for 16 vertices) and double its size everytime you run out of space. This will limit the number of required reallocs.

Comments

0

Try these changes , it should work.

void addPointerToArray(Vertex v1, Vertex (*out)[], int *size)
{
    int newSize = *size;
    newSize++;

    *out = realloc(out, newSize * sizeof(Vertex));
    *out[(*size)] = v1;

    //  Update Size
    *size = newSize;
}

and call the function like

addPointerToArray(vertexToAdd, &out, size);
  • There is a simple way to fix these type of issue (you might already know this). When you pass a argument to a function, think what exactly goes on to the stack and then combine the fact that what ever changes you make to variables present on stack would vanish when come out the function. This thinking should solve most of the issues related to passing arguments.

  • Coming to the optimization part, picking the right data structure is critical to the success of any project. Like somebody pointed out above, link list is a better data structure for you than the array.

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.