Kudayar Pirimbaev
Kudayar Pirimbaev

Reputation: 1320

C: Dynamic memory allocation - error of output

I'm trying to write a simple program that takes specific input, dynamically allocates it, outputs it and frees. The thing is it does not output it properly. The input's style is the following:

The first line is the number of lines that I need to read - i.

Then there are i lines. On each line I read one word, then an integer n that shows how many integers will proceed next and then there come n integers.

For example,

2
yellow 2 32 44
green 3 123 3213 3213

Explanation:

1st line - there must come 2 lines.

2nd and 3rd line - word + number of integers + integers.

My attempt:

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

int main()
{
    int i, j;
    int n; /* n - number of words */
    char **words; /* words - array of keywords */
    int **data;
    scanf ("%d\n", &n);

    words = (char **) malloc (n * sizeof (char *));
    data = (int **) malloc (n * sizeof (int *));

    for (i = 0; i < n; ++i)
    {
        words[i] = (char *) malloc (sizeof (char));
        for (j = 0 ;; ++j)
        {
            words[i] = (char *) realloc (words[i], sizeof (char) * (j + 2));
            scanf ("%c", &words[i][j]);

            if (words[i][j] == ' ')
                break;
            else if (words[i][j] == '\n')
                --j;

        }

        words[i][j] = '\0';
        data[i] = (int *) malloc (sizeof (int));
        scanf ("%d", &data[i][0]);

        for (j = 0; j < data[i][0]; ++j)
        {
            data[i] = (int *) realloc (data[i], sizeof (int) * (j + 2));
            scanf ("%d", &data[i][j]);
        }
    }

    for (i = 0; i < n; ++i)
    {
        printf ("%s ", words[i]);
        printf ("%d ", data[i][0]);
        for (j = 0; j < data[i][0]; ++j)
        {
            printf ("%d ", data[i][j]);
        }
        printf ("\n");
    }

    for (i = 0; i < n; ++i)
    {
        free (words[i]);
        free (data[i]);
    }
    free (words);
    free (data);
    return 0;
}

Upvotes: 0

Views: 158

Answers (1)

autistic
autistic

Reputation: 15632

data = (int **) malloc (n * sizeof (char *)); This doesn't make sense... The return value points to an int *, yet you're allocating in multiples of sizeof (char *). These two aren't required to have the same representation, which implies that they're not required to have the same width. See this page for more info. PS: Don't cast malloc. While you're there, read the rest of the website. It'll prevent you from encountering future common problems. In the mean time, I'll assume you meant data = malloc(n * sizeof *data);.

n, by the way, should probably be a size_t instead of an int. To recieve a size_t using scanf, use the %zu format specifier. An example of this is provided below.


    data[i] = (int *) malloc (sizeof (int));
    scanf ("%d", &data[i][0]);
    for (j = 0; j < data[i][0]; ++j)
    {
        data[i] = (int *) realloc (data[i], sizeof (int) * (j + 2));
            scanf ("%d", &data[i][j]);
    }

In this poorly indented example of code (which nobody wants to read, because it's poorly indented), a problem exists. The problem would normally go undetected because nobody wants to read it until it's correctly formatted, unnecessary casts are removed and consideration is given to the silly logic it presents.

The loop is supposed to end when j == data[i][0]. In the first iteration of the loop, data[i][0] changes, so the condition for the loop changes. Hence, this loop isn't doing what you want it to do. Perhaps you meant to write something like this:

    size_t count;
    /* Note how scanf returns a value, and when that value isn't 1 an assertion error
     * is raised? An exercise for you is to get that assertion error to raise, or read
     * the manual... */
    assert(scanf("%zu", &count) == 1);

    /* Note how malloc doesn't need a cast? */
    data[i] = malloc(count * sizeof data[i][0]);
    for (j = 0; j < count; ++j)
    {
        /* Note how count never changes, in this loop? */
        assert(scanf("%d ", &data[i][j]) == 1);
    }

While we're on this topic, you'll notice that I added a space to the end of the last scanf format string. That space consumes as much whitespace as possible from stdin. The reason for this is, presumably for the same purpose as the broken code in your previous loop, to read and discard any '\n' characters before reading the next item "word":


    words[i] = (char *) malloc (sizeof (char));
    for (j = 0 ;; ++j)
    {
        words[i] = (char *) realloc (words[i], sizeof (char) * (j + 2));
        scanf ("%c", &words[i][j]);
        if (words[i][j] == ' ')
            break;
        else if (words[i][j] == '\n')
            --j;
    }
    words[i][j] = '\0';

The possibility for leading '\n' characters is now virtually eliminated, with the exception of the user maliciously pressing enter without entering a word. malloc casts removed, and a more sensible allocation algorithm in vision, I presume you meant:

    size_t j = 0;
    words[i] = NULL;
    for (int c = getchar(); c >= 0; c = getchar()) {
        /* Reallocate when j is a power of two, eg: 0, 1, 2, 4, 8, 16...
         * ... and double the size of the buffer each time
         */
        if (j & (j - 1) == 0) {
            char *temp = realloc(words[i], j * 2 + 1);
            /* hint: Check *all* return values */
            assert(temp != NULL);
            words[i] = temp;
        }

        if (strchr(" \n", c) == NULL) { break; }
        words[i][j] = c;
        j++;
    }

    words[i][j] = '\0';

Upvotes: 1

Related Questions