Thread: Reassigning pointer within a function

Why don't you realloc smarty->array, rather than recreating a whole new container?

Originally Posted by Rorsh

first I allocate room for only 1 string(only pointers, not memory for strings)...

This sounds wrong: you have a parameter named length that is assigned to the member named capacity. It follows that you should allocate space for length (or capacity) number of pointers. Actually, it would probably be better to rename length to capacity since length sometimes means size, or could be misinterpreted as meaning the maximum length of a string in the array.

Also, there is no need to cast the return value of malloc. I suggest that instead of:

Code:

SmartArray* SA = NULL;

//there are checks for malloc failure but I omitted them now to make the code clearer
SA = (SmartArray*)malloc(sizeof(SmartArray));

You go with something like:

Code:

SmartArray* array = malloc(sizeof(*array));
// then do the check for malloc failure

Originally Posted by Rorsh

This function should insert strings into the array:

It would be good to specify: insert meaning append, prepend, or something like insert at a given location or in sorted order? Looking at the code, this insertion function appends a copy of the given string. I suggest that you make use of name prefixes for your functions, so instead of createSmartArray, you would have say, SmartArray_create, then instead of put you would have say, SmartArray_append.

Next, this looks strange:

Code:

if((strlen(str) + getCurrentLength(smarty)) >= smarty->capacity){
    smarty = expandSmartArray(smarty, smarty->capacity*2 + 1);
}

Why does the length of the string provided matter? You should expand when the capacity is about to exceed the size. I would have written:

Code:

if (smarty->size == smarty->capacity)
{
    smarty = SmartArray_expand(smarty, smarty->capacity * 2 + 1);
}

Now, instead of expanding the smart array before you are ready to insert, I would have prepared a copy of the string for insertion:

Code:

char* new_str = malloc(strlen(str) + 1);
if (new_str == NULL)
{
    printf("Couldn't allocate space for new string\n");
    return NULL;
}
strcpy(new_str, str);

and then expand, and finally append and return:

Code:

smarty->array[smarty->size++] = new_str;
return new_str;

I note that your expansion function has to consider the case where realloc returns a null pointer, so you may have to consider this in the calling code too.

Originally Posted by Rorsh

Here is my expandSmartArray function:

Since you designed the function to do nothing if the new capacity is not greater than the old capacity, it seems to me that you have over-complicated the code. You only need one realloc to increase the capacity. No additional malloc calls for the strings are needed since the size does not change. Likewise, you don't need to destroy anything.

Sorry for the double post. I did some changes, and now Expand looks like this:

Code:

SmartArray *expandSmartArray(SmartArray *smarty, int length){
    if(length <= smarty->capacity)
        return smarty;


    smarty->capacity = length;


    printf("Expanded SmartArray to size %d\n", smarty->capacity);


    return smarty;
}

Code:

char *put(SmartArray *smarty, char *str){
    if(smarty == NULL || str == NULL)
        return NULL;


    if((strlen(str) + getCurrentLength(smarty)) >= smarty->capacity){
        smarty = expandSmartArray(smarty, smarty->capacity*2 + 1);
    }


    smarty->array[smarty->size] = (char*)malloc(sizeof(char)*(strlen(str) + 1));


    if(smarty->array[smarty->size] == NULL)
    {
        printf("Couldn't allocate space for new string\n");
        return NULL;
    }




    strcpy(smarty->array[smarty->size], str);  //it crashes on this line, but I can't figure out why... It has worked for first 7 lines in my file, and crashes on 8th


    smarty->size += 1;


    return smarty->array[smarty->size - 1];
}

Output I get is:

Code:

Created new SmartArray of size 10
Expanded SmartArray to size 21
Expanded SmartArray to size 43
Expanded SmartArray to size 87

And then it crashes... It has already passed expanding two times, so that doesn't produce the crash, but I can't see what does. Any ideas?

Originally Posted by Rorsh

capacity is total number of characters in all strings (basically take all strings in the array and sum their lengths strlen(array[0]) + strlen(array[1]) + ....).

That is not useful unless you want to do a different approach to allocating memory: allocate space for pointers, then allocate space for one big block, then have the pointers point to within the big block. Only two allocations needed (and it may even be possible to make it one, though with some added complexity, in my opinion). However, if you then want to expand an individual string, things become complicated and possibly quite slow. Also, realloc does not always reallocate in-place, so you would need to handle the possibility that the pointers would no longer point to valid locations, so if your strings are of differing lengths, you might have to save their lengths before re-allocating, so you can restore them afterwards in the new locations.

If you want to allocate space for each string individually, then it is more useful for capacity to mean "number of pointers for which space has been allocated", i.e., "number of strings that can be stored without having to expand".

Originally Posted by Rorsh

Now, if the new capacity is less or equal to the new capacity that means that that array will not be expanded in any way, so I just return, otherwise I tired to copy the contents to the new array (totaly new Struct object), but maybe it would just be better if I incremented value of capacity to new capacity("length" in function declaration)?

Copying to a new struct object is a waste of time and space. You can modify the current struct object instead.

Originally Posted by Rorsh

Can you please elaborate on this a bit more?

Let's not go into this in detail just yet. I suggest that you first work on a solution where capacity means "number of strings that can be stored without having to expand", and then you allocate space for the pointers, and also individually for each string. If you cannot do that, chances are you will run into bigger problems trying to work with just two allocations.

Since your realloc worked without a cast, you can now remove the entirely redundant casts on your malloc calls.

Also, you should call realloc like so

Code:

void *temp = realloc(smarty->array, sizeof(char*)*length);
if ( temp != NULL ) {
    smarty->array = temp;
}

If realloc fails, you still have the original memory allocated. So you need to preserve that pointer until you know the reallocation has succeeded.