Thread: Realloc Strings Array

What error?
Also, count is uninitialized.

Try using an initial malloc line before the while loop where you call realloc. (I think you have to malloc before you can realloc). Unless, of course, palabras was malloced before you passed it to loadWords (I'll get to that at the end).

As Elysia points out, count is uninitialized so probably much more than zero to start with, which will be a significant problem. Then I think you want to concatenate buff onto secbuff in the while loop. You can use strlen instead of sizeof in realloc if the words may be different sizes, and error check:

Code:

int count=0, int len=1;
char *secbuff=malloc(1);      // that one byte will be the null terminator
secbuff[0]='\0'                // otherwise the first strcat will segfault
if (secbuff==NULL) exit (-1);
[...]
while(feof(diccionario)==0){
	fgets(buff, 6, diccionario);
        len+=strlen(buff);
	if ((secbuff = realloc(secbuff, len))==NULL) exit (-1); // however you would deal with this
        strcat(secbuff,buff);
	count++;
}
palabras=secbuff;

Notice that i didn't use palabras in the loop, since you are setting it to point to secbuff afterward anyway. Realize that when you used secbuff=realloc(palabras...), which is fine, that secbuff then points to palabras, so there was no point in the subsequent palabras=secbuff.

I don't actually know where palabras came from, what could be in it, or if it was already malloced, but I'm assuming "none of the above" since that will get thrown away by doing all this (in the original, and in my corrected variant).

Some people may tell you it is bad to call realloc with a return pointer that is the same as the one passed, secbuff=realloc(secbuff...). Really, this is only a problem if realloc fails, and if realloc fails your computer is out of memory and probably about to crash, so you might as well just exit (or at least "return -1" out of the function).

Of course, the list of words in palabras will not have any spaces between them this way! You will have to find a solution to that. Also, if you know how many words there are, or can find out before you call loadWords, you could pass in a 2D char* (a list of strings) and use that. This actually simplifies the memory allocation:

Code:

char *palabras[20];          // 20 words max
int X=loadWords("\folder\there\diccionario",palabras,20)

int loadWords(char *path, char **palabras, int max){
	FILE *diccionario;
	diccionario = fopen(path,"r");
	int count=0;
	char buff[7];
	printf("Empezando a cargar dic..");
	while(feof(diccionario)==0 && count<max){
		fgets(buff, 6, diccionario);
                if ((palabras[i]=malloc(strlen(buff)+1))==NULL) exit (-1);
		strcpy(palabras[i],buff);
		count++;
	}
	if(fclose(diccionario)!=0){
		printf("Error cerrando dic");
	}
	printf("Palabras cargadas");
	return count;
}

I'm not sure what you mean by unfixed list. Perhaps this, which is almost the same (but you can get rid of all your buffers):

Code:

char palabras[20][7];          // 20 words max
int X=loadWords("\folder\there\diccionario",palabras)

int loadWords(char *path, char palabras[20][7]){
	FILE *diccionario;
	diccionario = fopen(path,"r");
	int count=0;
	printf("Empezando a cargar dic..");
	while(feof(diccionario)==0){
		fgets(palabras[i], 6, diccionario);
                count++;
	}
	if(fclose(diccionario)!=0){
		printf("Error cerrando dic");
	}
	printf("Palabras cargadas");
	return count;
}

Here there's no need for malloc at all. However, you cannot vary the list size.

If you do use malloc in the function, remember that palabras can and should be freed at some point, either:

free(palabras);

or

for (i=0; i<X; i++) free(palabras[i]);

Remember, X was the return value from loadWords which should be the number of actual allocations.

If by "unfixed", you meant a list whose number of elements could be set, changed, and/or increased (maybe, palabras[][] or **palabras) as far as I know, you can't do that in C.

However, I only got this stuff straightened around for myself last week, so I could be wrong

You should not use feof as a loop control, either. The faq on the board has an explanation for that.

1. check return values before use (fopen, malloc etc could fail)
2. do not use feof to control loop - read FAQ
3. at the beginnign you have only one struct allocated, pointer[count] when count == 1 is out of bounds access
4. if realloc succeded - move your pointer to the new location - if it is failed - free old buffer
5. declare your path as const char* - you are not changing it in the function
6. pass your "struct _palabras *pointer" argument by pointer (you need pointer to pointer here) if you want the calling function to see the changes you made to the var

What vart means in simple terms:

Code:

T* p = malloc(...);
T* temp = realloc(p, ...);
if (temp)
    p = temp;
else
    free(p);

Originally Posted by lautarox

Is it possible to use reallocate like 1600 times?

Yes, but it is terribly inefficient to reallocate character by character. It would be far more efficient to keep track of both size (the number of characters in use) and capacity (the total number of characters allocated), then double the capacity everytime the size needs to exceed the current capacity. With such an approach you only need to reallocate 10 or 11 times instead of 1600 times. When you consider that memory allocation is an expensive operation, this could be a huge improvement in efficiency.

> Some people may tell you it is bad to call realloc with a return pointer that is the same as the one passed, secbuff=realloc(secbuff...).
> Really, this is only a problem if realloc fails, and if realloc fails your computer is out of memory and probably about to crash,
> so you might as well just exit (or at least "return -1" out of the function).
Really?
It sure will crash if you trash the pointer, and are then in the position of NOT being able to free the 100MB of memory you just leaked.

If you leak it because you ran out, then you probably had a lot of it to begin with.

Do the right thing, free up some memory at the moment of potential failure and save the day.

*shakes head*
This seems to be a hard message for you to understand.