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.

• Mat: This is C, you don't need the cast at all. (Not needed for malloc either.)
• Ziezi: @Barry , I appreciate your remarks, I will skip the wrapping function increase_buffer_size() and include an additional line after the call torealloc(), to check whether buffer_pointer is the nullptr, together with some error message, if it is.
• Ziezi: @Mat, it gets underlined in red if I don't cast to char*explicitly.
• Mat: @simplicisveritatis: then you're not using a C compiler, or your IDE is trying to be too helpful and ends up being counter-productive. See stackoverflow.com/questions/605845/….
• Mat: @simplicisveritatis: that cast offers zero type safety. But feel free to keep it - in this specific case it doesn't hurt. Keep in mind the others though (stated in the SO link).

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