C: Dynamic memory allocation - error of output

Question

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;
}

What is sizeof (char)? Hint: How many chars fit in a char? What's the point in multiplying or dividing by 1? — autistic
– autistic, Commented Apr 14, 2013 at 11:51
Don't cast malloc.. Please adjust your indentation so that it's formatted consistently. — autistic
– autistic, Commented Apr 14, 2013 at 11:54

autistic · Accepted Answer · 2013-04-14 12:45:16Z

1

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';

edited Apr 14, 2013 at 12:45

answered Apr 14, 2013 at 12:19

autistic

15.8k4 gold badges40 silver badges81 bronze badges

Sign up to request clarification or add additional context in comments.

10 Comments

Kudayar Pirimbaev Over a year ago

my fault with indentation - in the temporary version of question that is shown on the bottom where i create it it was giving false indentation, so i was adding some of tab characters, then it turned to an error

Kudayar Pirimbaev Over a year ago

thanks, silly mistake about allocating to character pointers, was brushing up my C, haven't written code in it for ages

Kudayar Pirimbaev Over a year ago

by the way, why have you included "while ()" at the end of for loop?

autistic Over a year ago

@KudayarPirimbaev I'm glad you understood that. SIZE_MAX is required to be (size_t) -1 which is represented in binary entirely using 1's. The algorithm used here is neat because it uses numbers that are also represented in binary entirely using 1's. That makes it easy to tell if the object you have allocated is already the maximum size, allocate the next object to be a continuation and continue as though it's simply a new item.

autistic Over a year ago

@KudayarPirimbaev Better yet is an approach to program without buffers. I gather you hyaven't considered using an associative array to store the numbers as data, using the words "green" and "yellow" as keys. Some associative arrays don't require storage of their keys. For example, consider a hash table; The user could input as many characters as he/she wants up until he/she presses enter, and your program would read each keystroke using getchar() and calculate an index to an array on the fly.

|

Collectives™ on Stack Overflow

C: Dynamic memory allocation - error of output

1 Answer 1

10 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

10 Comments

Your Answer

Sign up or log in

Post as a guest

Related