Read characters from stdin into an resizeable array

Question

As first steps in C, I'm trying to write a program that reads from stdin into an array allocated on the free store, until an exclamation mark ! is entered. The array should be extended to prevent overflow.

Here is what I've written:

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

char* increase_buffer_size(char *buff_ptr, size_t multiplier);

//==================================================================
int main () {
    // initial buffer size
    size_t buffer_size = 32;

    // allocate 32 bytes of heap memory
    char *buffer_pointer = (char*) malloc(buffer_size);

    // input loop variables
    char input_var;
    size_t counter = 0;
    char sentinel = '!';

    // input loop
    printf("Type input\n>>");
    while (input_var = getchar()) {

        // check termination condition
        if (input_var == sentinel) break;

        // check capacity and double it if maximum is reached
        if (counter == buffer_size - 1) buffer_pointer = increase_buffer_size(buffer_pointer, buffer_size *= 2);

        // populate buffer
        buffer_pointer[counter++] = input_var;
    }

    getchar();

    // print input values
    printf("Buffer content: \n");
    for (auto i = 0; i < counter; ++i) printf("%c %c", buffer_pointer[i], ' ');

    getchar();
}

//==================================================================

char* increase_buffer_size(char *buff_ptr, size_t new_size) {

    char* new_ptr = (char*) realloc(buff_ptr, new_size);

    return new_ptr;
} 

As I already mentioned these are my first steps in C, so I was wondering:

Is the above code written in accordance with C conventions?

Is there anything that can be skipped or written better?

Answer 1

Unnecessary function

The only difference between increase_buffer_size() and realloc() is that the former returns a char* and the latter returns a void*. But you can just manually do the cast anyway, which will still be shorter to type.

buffer_pointer = (char*)realloc(buffer_pointer, buffer_size *= 2);

Avoid putting lots of things on on one line

Your reallocation line does three things: checks if you need to reallocate, doubles the buffer size, and reallocates. It'd be much better to just split that up. Don't write the bodies of your if statements on the same line either, unless the body is trivial (e.g. return;). Using {}s is a good habit to get into:

// check capacity and double it if maximum is reached
if (counter == buffer_size - 1) {
    buffer_size *= 2;
    buffer_pointer = (char*)realloc(buffer_pointer, buffer_size);
}

What if realloc fails?

It's possible for it to return a null pointer after all.

Answer 2

A few general style thoughts:

Try to declare all of your variables at the top of the function, before any code (in your case, wait to set buffer_pointer until after the rest of the declarations). This was mandatory in the original versions of the C language standard, but is no longer strictly necessary in the most recent versions. Different compilers support different versions of the C standard, so you're generally safest if you try and do it the old way as much as possible.

When declaring a function that takes no parameters, use (void) instead of leaving the parentheses empty. This makes your intentions clear to someone else reading your code. Also, while most compilers will silently treat () as (void), that behavior is not universal and is not safe to rely on. It's nearly always better to be explicit than to trust that the compiler (or someone who has to maintain your code in the future) will be able to infer what you meant.

Code with side effects (like counter++) should be on a line by itself. Currently, that line of code does two things: it increments counter and it uses counter as an index into the array. Which of these two things happens first? It's not immediately obvious, but it's critical to understanding the way that the code behaves. Instead, writing that line as:

buffer_pointer[counter] = input_var;
counter++;

makes the code's intentions obvious, and makes it impossible to introduce subtle, hard-to-find bugs like accidentally typing ++counter instead of counter++. The same thought applies to the buffer_size *= 2 statement inside the call to increase_buffer_size().

In your for loop, you don't need to use the auto keyword. Actually, you shouldn't ever need to use auto. Automatic storage duration is the default. The keyword only exists for historical reasons. You do, however, need to specify a variable type for i. Since you are comparing i to counter, it's generally best to use the same variable type as counter. As mentioned earlier, it would be safer to declare i at the top of the function along with the other variables.

Your printf inside the loop seems to be overly complicated. The format string is "%c %c", which prints a character, followed by a space, followed by another character. You have the second character hard-coded to a space. Why not just use "%c "?

As far as functionality is concerned, you may want to re-think the way that you are enlarging the buffer. You are doubling the size of the buffer each time you run out of space. This probably works fine for short input sequences, but your memory usage can get out of hand very quickly. It's probably safer to add memory in fixed-size chunks so that memory usage increases at a controllable rate. For example:

#define CHUNK_SIZE  32

...

size_t buffer_size = CHUNK_SIZE;

...

if (counter == (buffer_size - 1)) {
    buffer_size += CHUNK_SIZE;
    buffer_pointer = increase_buffer_size(buffer_pointer, buffer_size);
}