Thread: Dynamicaly allocating memory for unknown number of strings of unknown sizes in c

Hello everyone nice to meet you , new member on this site.I would apreciate your help to finish a program i am working on , what i am trying to do is allocate memory for an unknown number of strings of unknown sizes.The problem is that i am missing something in my logic with pointers and the program crashes (probably i am not allocating righly).Thank you in advance and excuse my english.https://cboard.cprogramming.com/images/icons/icon7.png

Here is the code:

Code:

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


char* GetString();
void StoreStrings(char ***storage,int *size);


int main()
{
    char **strings = NULL;
    int size,i;


    StoreStrings(&strings,&size);


    for(i=0;i<size;i++)
    {
        printf("%s\n",strings[size]);
    }


    system("PAUSE");
    return 0;
}


char* GetString()
{
    char *string = NULL,c;
    int size = 1;


    string = (char*)malloc(size+1);
    string[size-1] = '\0';


    while((c = getchar()) != '\n')
    {
        string[size-1] = c;
        size++;


        string = (char*)realloc(string,size+1);
        string[size-1] = '\0';
    }


    return string;
}


void StoreStrings(char ***storage,int *size)
{
    char *string = NULL;


    *size = 0; //number of strings set to 0


    *storage = (char**)malloc((*size+1) * sizeof(char*)); //allocating memory for the number of the strings
    *storage[*size] = (char*)malloc(sizeof(char)); //allocating memory for the string size of the 1st string (i use malloc here because i have read that you should call malloc first and then use realloc)


    string = GetString(); //reading a string


    while(strcmp(string,"") != 0) //if user gives empty string storing proccess ends
    {
        *storage[*size] = (char*)realloc(*storage[*size],(strlen(string)+1) * sizeof(char)); //reallocating memory for the string (+1 is for '\0')


        strcpy(*storage[*size],string); //storing the string


        (*size)++; //updating the number of strings


        *storage = (char**)realloc(*storage,(*size+1) * sizeof(char*)); //reallocating memory for the updated number of strings


        string = GetString(); //reading new string
    }


    free(string);
}

Its above your last reply.

Originally Posted by jaspar

Program is working!

It might seem to be working but your code is riddled with errors, inefficiencies and memory leaks.

Originally Posted by algorism

It might seem to be working but your code is riddled with errors, inefficiencies and memory leaks.

Could you please point out those errors , inefficiencies and memory leaks,so i can learn more about c and of course try to fix them.

A few off the top of my head:

You allocate more memory and strcpy the string into it (and then don't free the original string). You should just assign the original string pointer to the storage element.

You shouldn't be realloc'ing one element at a time.

You haven't removed the casts for malloc and realloc. If your compiler complains then you are compiling as C++, not C. That's a major problem!

You should accept the return value of realloc into a temporary variable. That way if it returns NULL you don't lose the original pointer value that you passed in.

Why pass size into the function? Why not make size local and return it? Or perhaps pass in &size but make storage a local char** and return it.

getchar returns an int, not a char. You should be testing for a possible EOF. EOF is an int. Comparing a char to EOF is an error (and will fail is char is unsigned).

sizeof(char) is always 1.

Originally Posted by algorism

A few off the top of my head:

You allocate more memory and strcpy the string into it (and then don't free the original string). You should just assign the original string pointer to the storage element.

You shouldn't be realloc'ing one element at a time.

You haven't removed the casts for malloc and realloc. If your compiler complains then you are compiling as C++, not C. That's a major problem!

You should accept the return value of realloc into a temporary variable. That way if it returns NULL you don't lose the original pointer value that you passed in.

Why pass size into the function? Why not make size local and return it? Or perhaps pass in &size but make storage a local char** and return it.

getchar returns an int, not a char. You should be testing for a possible EOF. EOF is an int. Comparing a char to EOF is an error (and will fail is char is unsigned).

sizeof(char) is always 1.

Thank you , you and salem really helped me.I am gonna study what you told me and try to improve my code.I would really be grateful if you could write a program (if you have time and if you want to) with the same thinking as mine and post it here so i can see how you would code this program.If you dont , i am still much grateful to both of you.Thank you again for the help!Cheers!https://cboard.cprogramming.com/images/icons/icon10.png

This is pretty quick and dirty, but something like this.

Code:

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

#define INITIAL_STRING_SIZE        10
#define INITIAL_STRING_ARRAY_SIZE  10

char*  GetString       ();
char** StoreStrings    (int *size);
void   DeleteStrings   (char ***strings, int size);
void*  realloc_or_free (void *in, size_t sz);
void   die             (const char *msg);

int main() {
    char **strings = NULL;
    int size, i;

    strings = StoreStrings(&size);

    for (i = 0; i < size; i++)
        printf("%s\n", strings[i]);

    DeleteStrings(&strings, size);

    return 0;
}

char *GetString() {
    int c, size = 0, allocated = INITIAL_STRING_SIZE;
    char *str;

    str = malloc(allocated);
    if (str == NULL)
        return NULL;

    while ((c = getchar()) != EOF && c != '\n') {
        if (size == allocated - 1) {
            str = realloc_or_free(str, allocated *= 2);
            if (str == NULL)
                return NULL;
        }
        str[size++] = c;
    }
    str[size] = '\0';

    /* Fit to size and return. */
    return realloc_or_free(str, size + 1);
}

char **StoreStrings(int *size) {
    int allocated = INITIAL_STRING_ARRAY_SIZE;
    char **strings, **temp;

    *size = 0;
    strings = malloc(allocated * sizeof(char*));
    if (strings == NULL)
        die("StoreStrings 1");

    printf("Enter strings. Enter an empty string to quit.\n");

    while (1) {
        char *str = GetString();
        if (str == NULL) {
            DeleteStrings(&strings, *size);
            die("StoreStrings 2");
        }

        if (str[0] == '\0') {
            free(str);
            break;
        }

        if (*size == allocated) {
            temp = realloc(strings, (allocated *= 2) * sizeof(char*));
            if (temp == NULL) {
                DeleteStrings(&strings, *size);
                die("StoreStrings 3");
            }
            strings = temp;
        }

        strings[(*size)++] = str;
    }

    /* Fit to size. */
    temp = realloc(strings, *size * sizeof(char*));
    if (temp == NULL) {
        DeleteStrings(&strings, *size);
        die("StoreStrings 4");
    }

    return temp;
}

void DeleteStrings(char ***strings, int size) {
    int i;
    for (i = 0; i < size; i++)
        free((*strings)[i]);
    free(*strings);
    *strings = NULL;
}

/* Frees original storage if realloc fails. */
void *realloc_or_free(void *in, size_t sz) {
    void *p;
    p = realloc(in, sz);
    if (p == NULL)
        free(in);
    return p;
}

/* Print error message and exit. */
void die(const char *msg) {
    fprintf(stderr, "Error: %s\n", msg);
    exit(EXIT_FAILURE);
}

Thank you algorism , i am gonna study it now and try to better my coding abilities.You deserve a medal. :P