arrays question

[Eric]

I think this is correct but i wanted to be sure, I'm wanting someone to tell
me if I'm doing the array allocation and deallocation correctly.

int i, wc = 0;
char *vptr;
char **Array;

Array = malloc(sizeof(char **));

while(some condition)
{
vptr = some char array with a length 1 and 20 bytes
Array = realloc(Array, sizeof(char**)*wc+1);
Array[wc] = malloc(strlen(vptr)+1);
strcpy(WordArray[wc], vptr);
wc++;
}
// then later on
for(i=0; i<wc; i++)
free(Array);
free(Array);


go for liftoff?
Thanks
Eric

 

[brandon.craig]

I think this is correct but i wanted to be sure, I'm wanting someone to tell
me if I'm doing the array allocation and deallocation correctly.

int i, wc = 0;
char *vptr;
char **Array;

Array = malloc(sizeof(char **));

while(some condition)
{
vptr = some char array with a length 1 and 20 bytes
Array = realloc(Array, sizeof(char**)*wc+1);
Array[wc] = malloc(strlen(vptr)+1);
strcpy(WordArray[wc], vptr);
wc++;
}
// then later on
for(i=0; i<wc; i++)
free(Array);
free(Array);

hi eric,

i'm fairly new using c, so hopefully the seasoned guys on here will
just correct any errors i make in this post without flaming either of
us.

Array = realloc(Array, sizeof(char**)*wc+1);

although that appears to me to be valid, i suspect that realloc could
be fairly slow. instead of calling it every time through the loop, you
might want to allocate extra space, skip over the realloc until you've
consumed all your available space, then give back what you don't use
after the while() loop.

more importantly, if realloc fails, it will return NULL. so Array will
no longer point to your data. immediately following the realloc, you
dereference Array, which would be bad if it was NULL. you also will
have no way to free the memory it used to point at. a temporary
pointer would solve that problem.

so something like this might be a little safer and a little faster:

int i = 0, res_size = 1;
void *tmp;

array = malloc(sizeof(...) * res_size);

while(...)
{

if (i == res_size)
{
res_size *= 2; /* double array size when space is needed. */

tmp = realloc(array, sizeof(...) * res_size);
if (tmp == NULL)
{
/* handle error, free array and its elems if necessary, exit,
etc. */
}

array = tmp;
}

array = malloc(...);
strcpy(array, vptr);
i++;
}

realloc(array, sizeof(...) * i) /* shrink back down to final size */

the array doubling may be too aggressive in some cases. i just used it
as an example so you'd get the idea.

for(i=0; i<wc; i++)
free(Array);
free(Array);

i believe that is the correct way to free everything.

brandon

 

[Keith Thompson]

I think this is correct but i wanted to be sure, I'm wanting someone to tell
me if I'm doing the array allocation and deallocation correctly.

int i, wc = 0;
char *vptr;
char **Array;

Array = malloc(sizeof(char **));

Array is a pointer-to-pointer-to-char. If you want to allocate enough
space for a single pointer-to-char (to which Array will point), you
need to use sizeof(char*), not sizeof(char**). The usual idiom for
this is:

Array = malloc(sizeof *Array);

(sizeof(char*) is likely to be the same as sizeof(char**) on most
systems, so this bug could be difficult to detect.

And, of course, you should always check whether malloc() succeeded.

while(some condition)
{
vptr = some char array with a length 1 and 20 bytes
Array = realloc(Array, sizeof(char**)*wc+1);

I think the second argument is wrong. Think about operator
precedence.

Array[wc] = malloc(strlen(vptr)+1);
strcpy(WordArray[wc], vptr);

This should be Array[wc], not WordArray[wc], right?

wc++;
}

Since you're not showing us real code, it's hard to tell exactly
what's going on, but I think you're calling realloc() each time you
increase the array length by one. This is likely to cause heap
fragmentation. Try increasing the array size in bigger increments.

[snip]

It's easier to help if you show us real code, or explain what you're
trying to do, or, ideally, both.

 

[santosh]

I think this is correct but i wanted to be sure, I'm wanting someone to tell
me if I'm doing the array allocation and deallocation correctly.

int i, wc = 0;
char *vptr;
char **Array;

Array = malloc(sizeof(char **));

This is probably wrong. You're assigning to Array, (which is a object
of type pointer to pointer to char), sizeof(char **) bytes. This would
make Array point to the start of a region of memory large enough to
hold on variable of type char **, which is what Array already is.
Furthermore if you change the type of Array in the future, then the
value returned by the sizeof will not be correct. You've also failed to
check weather malloc() has succeeded or not.

In fact given rest of your code, I don't think this statement is needed
at all.

while(some condition)
{
vptr = some char array with a length 1 and 20 bytes

Not as you've declared above. It's a pointer to type char. It won't
automatically point to anywhere. I'm assuming you've initialised it to
the start of an array in code you've not shown above.

Array = realloc(Array, sizeof(char**)*wc+1);

Many mistakes here. Firstly always pass realloc() a *copy* of the
pointer. If realloc() fails to resize the memory block, it will return
a null pointer *without* altering the original memory block, in which
case the assignment above means that you've lost access to it. Again
you've failed to check the return value of realloc(). Again hard-coding
the type of Array into the sizeof operator is inflexible. The following
form of the statement may be better:

/* Copy the return value of realloc() into a temp. pointer */
tmp_ptr = realloc(Array, sizeof **Array * wc + 1);
if(tmp_ptr)
Array = tmp_ptr;
else
free(Array);
/* Do something else */

Array[wc] = malloc(strlen(vptr)+1);

Again check for malloc() failure.

strcpy(WordArray[wc], vptr);

Are you sure this is what you want? You haven't included the
declaration of WordArray so I can't say anything definite at this
point. Always include the declaration of objects shown in the code, or
atleast mention their types.

go for liftoff?

Not quite. First make all your flight checks.

 

[zhousqy]

I think this is correct but i wanted to be sure, I'm wanting someone to tell
me if I'm doing the array allocation and deallocation correctly.

int i, wc = 0;
char *vptr;
char **Array;

Array = malloc(sizeof(char **));

while(some condition)
{
vptr = some char array with a length 1 and 20 bytes
Array = realloc(Array, sizeof(char**)*wc+1);
Array[wc] = malloc(strlen(vptr)+1);
strcpy(WordArray[wc], vptr);
wc++;
}
// then later on
for(i=0; i<wc; i++)
free(Array);
free(Array);


go for liftoff?
Thanks
Eric

What i want to say is that you should always remember to check the
return value of "malloc" or "realloc" .

 

[Chris Dollin]

I think this is correct but i wanted to be sure, I'm wanting someone to
tell me if I'm doing the array allocation and deallocation correctly.

This code presumably lives in some function, right?

int i, wc = 0;
char *vptr;

You can move this declaration to inside the while loop.

char **Array;

Array = malloc(sizeof(char **));

You can combine the declaration and the initialisation.

You have not declared malloc (by including <stdlib.h>).

malloc might have failed - you should check.

while(some condition)
{
vptr = some char array with a length 1 and 20 bytes
Array = realloc(Array, sizeof(char**)*wc+1);

That looks wrong.

realloc might have failed - you should check. (And if it
failed, you need to keep hold of the old value of `Array`.)

The +1 is incorrect. Did you mean `(wc + 1)`? Otherwise on
the first pass you're asking to realloc to 1 byte of store,
which is unlikely to be enough for a char**.

If you're using `realloc` like this, you should be able to
initialise `Array` to the null pointer.

Array[wc] = malloc(strlen(vptr)+1);
strcpy(WordArray[wc], vptr);

`WordArray`? Who's he?

wc++;
}
// then later on
for(i=0; i<wc; i++)
free(Array);
free(Array);


go for liftoff?
Thanks
Eric

 

[stathis gotsis]

Not as you've declared above. It's a pointer to type char. It won't
automatically point to anywhere. I'm assuming you've initialised it to
the start of an array in code you've not shown above.

Many mistakes here. Firstly always pass realloc() a *copy* of the
pointer. If realloc() fails to resize the memory block, it will return
a null pointer *without* altering the original memory block, in which
case the assignment above means that you've lost access to it. Again
you've failed to check the return value of realloc(). Again hard-coding
the type of Array into the sizeof operator is inflexible. The following
form of the statement may be better:

/* Copy the return value of realloc() into a temp. pointer */
tmp_ptr = realloc(Array, sizeof **Array * wc + 1);

Minor correction here, this should be:
tmp_ptr = realloc(Array, sizeof *Array * (wc + 1));

if(tmp_ptr)
Array = tmp_ptr;
else
free(Array);
/* Do something else */

Array[wc] = malloc(strlen(vptr)+1);

And this is correct as: sizeof **Array == sizeof(char) == 1.

 

[Eric]

I think this is correct but i wanted to be sure, I'm wanting someone to
tell me if I'm doing the array allocation and deallocation correctly.

int i, wc = 0;
char *vptr;
char **Array;

Array = malloc(sizeof(char **));

while(some condition)
{
vptr = some char array with a length 1 and 20 bytes
Array = realloc(Array, sizeof(char**)*wc+1);
Array[wc] = malloc(strlen(vptr)+1);
strcpy(WordArray[wc], vptr);
wc++;
}
// then later on
for(i=0; i<wc; i++)
free(Array);
free(Array);


go for liftoff?
Thanks
Eric

I will respond, but my machine is giving me fits and wont stay running long
enough to really respond. I did what you all said - its working well thanks
a million. I will respond in detail at some other time when my pc is
running correctly, sorry for this - its pissing me off, got to go i can see
its about to crash now.
Eric

 

[Eric]

I think this is correct but i wanted to be sure, I'm wanting someone to
tell me if I'm doing the array allocation and deallocation correctly.

int i, wc = 0;
char *vptr;
char **Array;

Array = malloc(sizeof(char **));

while(some condition)
{
vptr = some char array with a length 1 and 20 bytes
Array = realloc(Array, sizeof(char**)*wc+1);
Array[wc] = malloc(strlen(vptr)+1);
strcpy(WordArray[wc], vptr);
wc++;
}
// then later on
for(i=0; i<wc; i++)
free(Array);
free(Array);


go for liftoff?
Thanks
Eric

I will respond, but my machine is giving me fits and wont stay running
long enough to really respond. I did what you all said - its working well
thanks a million. I will respond in detail at some other time when my pc
is running correctly, sorry for this - its pissing me off, got to go i can
see its about to crash now.
Eric

sorry, for that, my other pc is having problems - anyway -
First - sorry for the half way pseudo code, i was trying to focus on what i
wanted to know. I implemented everything people mentioned and put the code
through the wringer and it works well.
What it was for was basically a routine to take a block of text, up to 4k in
length and take each word and store it in an array. Then i would reassemble
it in lines of approx 80 chars, with breaks at word boundaries and insert
some html tags at the start and end of each line.
If anyone is curious i will post the code
Again, thanks for the help, i really appreciate the critique.
Eric