A trivial command line utility for trimming whitespace from lines in C - follow-up 3

Question

The previous iteration at A trivial command line utility for trimming whitespace from lines in C - follow-up 2

Now my code looks like:

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

#define HELP_FLAG           "-h"
#define VERSION_FLAG        "-v"
#define FLAG_DESC           "%-5s"
#define INITIAL_BUFFER_SIZE 64

/*******************************************************************************
* This routine removes all leading and trailing whitespace from a string,      *
* doing that in-place and using only one pass over the string.                 *
*******************************************************************************/
static char* trim_inplace(char* start)
{
    while (isspace(*start))
    {
        ++start;
    }

    int whitespace_begin_index = -1;

    // At this point, we have dealt with leading whitespace.
    for (int index = 0; start[index]; ++index)
    {
        if (!isspace(start[index]))
        {
            whitespace_begin_index = -1;
        }
        else if (whitespace_begin_index == -1)
        {
            whitespace_begin_index = index;
        }
    }

    if (whitespace_begin_index != -1)
    {
        // Cut the trailing whitespace off.
        start[whitespace_begin_index] = '\0';
    }

    return start;
}

/*******************************************************************************
* Attempts to expand the line buffer. If succeeded, returns the pointer to the *
* line buffer. Otherwise NULL is returned.                                     *
*******************************************************************************/
static char* try_expand(char* buffer, int* p_buffer_length)
{
    *p_buffer_length *= 2;
    return realloc(buffer, *p_buffer_length);
}

/*******************************************************************************
* Processes a single line and handles everything needed for dealing with lines *
* of arbitrary length.                                                         *
*******************************************************************************/
static void process_line(char** p_buffer, int* p_buffer_length, FILE* file)
{
    int chars_read = 0;

    for (;;)
    {
        // The delta is for appending the next text chunk at correct position.
        int delta = chars_read > 0;
        char* ret = fgets(*p_buffer + chars_read - delta,
                          *p_buffer_length - chars_read + delta,
                          file);

        if (!ret)
        {
            return;
        }

        // Find out whether we have a newline character, which would imply that
        // we have an entire line read.
        for (int i = 0; i < *p_buffer_length; ++i)
        {
            if ((*p_buffer)[i] == '\n')
            {
                (*p_buffer)[i] = '\0';
                puts(trim_inplace(*p_buffer));
                return;
            }

        }

        chars_read = *p_buffer_length;
        char* new_buffer;

        // Once here, the current line does not fit in 'p_buffer'. Expand the
        // array by doubling its capacity.
        if (!(new_buffer = try_expand(*p_buffer, p_buffer_length)))
        {
            perror("Could not expand the line buffer");
            free(*p_buffer);
            exit(EXIT_FAILURE);
        }
        else
        {
            *p_buffer = new_buffer;
        }
    }
}

/*******************************************************************************
* Processes a file.                                                            *
*******************************************************************************/
static void process_file(char** p_buffer, int* p_buffer_length, FILE* file)
{
    while (!feof(file))
    {
        process_line(p_buffer, p_buffer_length, file);
    }
}

/*******************************************************************************
* If name contains directories, gets rid of them and returns a sole name of    *
* the executable file.                                                         *
*******************************************************************************/
static const char* get_short_program_name(const char* name)
{
    size_t len = strlen(name);
    const char* ret = name;

    for (size_t i = 0; i < len; ++i)
    {
        if (name[i] == '/')
        {
            ret = &name[i + 1];
        }
    }

    return ret; 
}

/*******************************************************************************
* Prints the help message and exits.                                           *
*******************************************************************************/
static void print_help(const char* program_name)
{
    printf("Usage: %s [" HELP_FLAG "] [" VERSION_FLAG "] "            \
           "[FILE1, [FILE2, [...]]]\n"                                \
           "    " FLAG_DESC " Print this help message and exit.\n"    \
           "    " FLAG_DESC " Print the version message and exit.\n"  \
           "    If no files specified, reads from standard input.\n",
           get_short_program_name(program_name),
           HELP_FLAG,
           VERSION_FLAG);
}

/*******************************************************************************
* Prints the version string.                                                   *
*******************************************************************************/
static void print_version()
{
    printf("trim 1.6180\n" \
           "By Rodion \"rodde\" Efremov 10.04.2015 Helsinki\n");
}


/*******************************************************************************
* Prints the erroneous flag.                                                   *
*******************************************************************************/
static void print_bad_flag(const char* flag)
{
    printf("Unknown flag \"%s\"\n", flag);
}

/*******************************************************************************
* Checks the flags.                                                            *
*******************************************************************************/
static void check_flags(int argc, char** argv)
{
    int c;

    while ((c = getopt(argc, argv, "vh")) != -1)
    {
        switch (c) 
        {
            case 'v':
                print_version();
                exit(EXIT_SUCCESS);
                break;

            case 'h':
                print_help(argv[0]);
                exit(EXIT_SUCCESS);
                break;

            case '?':
                print_bad_flag((char*) &optopt);
                exit(EXIT_FAILURE);
                break;

            default:
                abort();
        }
    }
}

/*******************************************************************************
* The entry point for a trivial line trimmer.                                  *
*******************************************************************************/
int main(int argc, char** argv)
{
    check_flags(argc, argv);

    int buffer_length = INITIAL_BUFFER_SIZE;
    char* buffer = malloc(buffer_length);

    if (argc < 2)
    {
        // If realloc changes the location of memory, we need to know this.
        process_file(&buffer, &buffer_length, stdin);
        fclose(stdin);
        free(buffer);
        return EXIT_SUCCESS;
    }

    for (int i = 1; i < argc; ++i)
    {
        FILE* file = fopen(argv[i], "r");

        if (!file)
        {
            perror("Error opening a file");
            return (EXIT_FAILURE);
        }

        process_file(&buffer, &buffer_length, file);
        fclose(file);
    }

    free(buffer);
}

I have done the following:

1. Boolean stuff removed as it is no longer used.
2. Whenever printing the help message, the actual name of the binary is used.
3. Used getopt for parsing command line arguments.
4. Fixed size_t versus int warnings.

Answer 1

1. Rather than while (!feof(file)) { process_line(p_buffer, p_buffer_length, file); }, consider having process_line() return a value that can be tested. fgets() returns NULL for at least 3 reasons: EOF, input error, scant buffer. Calling routine only looks for the common feof()

2. main() does not check for a NULL result in char* buffer = malloc(buffer_length);, yet later code dereferences that pointer also without checking.

3. Pedantically, isspace(*start)) is potential undefined behavior (UB) when char is signed. is...() functions take a int in the range of unsigned char or EOF. Solution: isspace((unsigned char) *start))

4. String array indexes as well as memory allocation work with type size_t and not int like this code employs. This impacts portability/correctness once the array size exceeds the minium of INT_MAX,SIZE_MAX.

5. In try_expand(), *p_buffer_length *= 2; does not check for potential overflow. return realloc(buffer, *p_buffer_length); does not check for memory allocation failure but leaves that for the calling code. IMO, the exit handling should be in try_expand().

6. void print_version() should be void print_version(void) so a complier properly complains if print_version() called with a parameter as in print_version("123")

Answer 2

Starting from the top…

The trim_inplace() function reads more naturally if you work with pointers instead of indexes. In particular, whitespace_begin_index is awkwardly named, and using -1 as a special value is less elegant than using NULL.

/*******************************************************************************
* This routine removes all leading and trailing whitespace from a string,      *
* doing that in-place and using only one pass over the string.                 *
*******************************************************************************/
static char* trim_inplace(char* start)
{
    // Skip leading whitespace
    while (isspace(*start)) {
        ++start;
    }

    // Cut off trailing whitespace
    char *cutoff = NULL;
    for (char *p = start; *p; p++)
    {
        if (!isspace(*p))
        {
            cutoff = NULL;
        }
        else if (!cutoff)
        {
            cutoff = p;
        }
    }
    if (cutoff)
    {
        *cutoff = '\0';
    }

    return start;
}

---

try_expand(), process_line(), and process_file() can all be greatly simplified. All of the code exists primarily to manage memory for reading lines of arbitrary length. Fortunately, on POSIX platforms, you can just use getline() to eliminate all of that drudgery. In fact, you don't even need to malloc() anything at all.

Moreover, the fact that you need a loop in process_line() to search for '\n' makes the implementation not a one-pass algorithm as you claim. (While doing line-oriented work requires scanning for the '\n', the getline() interface tells you how long the line was, whereas the fgets() interface makes you hunt for '\n' all over again.)

For readability, you should use size_t instead of int for a variable that indicates the size of a buffer.

/*******************************************************************************
* Processes a file.                                                            *
*******************************************************************************/
static void process_file(char** p_buffer, size_t* p_buffer_length, FILE* file)
{
    while (-1 != getline(p_buffer, p_buffer_length, file))
    {
        puts(trim_inplace(*p_buffer));
    }
}

---

Error messages should be printed to stderr so as not to contaminate stdout.

You've already hard-coded h and v in the check_flags() function, so you are committed to using those letters anyway. You might as well eliminate the HELP_FLAG, VERSION_FLAG, and FLAG_DESC macros to make the string more readable.

Instead of implementing get_short_program_name(), just call basename(). (And I would do it in check_flags() rather than print_help().)

The getopt() function automatically prints a diagnostic message when an unrecognized flag is encountered. If you wish to print_bad_flags() manually, then you should disable the redundant automatic message by setting opterr = 0.

Consider adding -l and -t flags to control whether to trim just leading whitespace or trailing whitespace.

/*******************************************************************************
* Prints the help message and exits.                                           *
*******************************************************************************/
static void print_help(const char* program_name, FILE *f)
{
    fprintf(f, "Usage: %s [-h] [-v] [FILE1, [FILE2, [...]]]\n"
               "    -h    Print this help message and exit.\n"
               "    -v    Print the version message and exit.\n"
               "    If no files specified, reads from standard input.\n",
               program_name);
}

/*******************************************************************************
* Prints the version string.                                                   *
*******************************************************************************/
static void print_version()
{
    printf("trim 1.6180\n" \
           "By Rodion \"rodde\" Efremov 10.04.2015 Helsinki\n");
}


/*******************************************************************************
* Prints the erroneous flag.                                                   *
*******************************************************************************/
static void print_bad_flag(const char* flag)
{
    fprintf(stderr, "Unknown flag \"%s\"\n", flag);
}

/*******************************************************************************
* Checks the flags.                                                            *
*******************************************************************************/
static void check_flags(int argc, char** argv)
{
    int c;

    opterr = 0;
    while ((c = getopt(argc, argv, "vh")) != -1)
    {
        switch (c) 
        {
            case 'v':
                print_version();
                exit(EXIT_SUCCESS);
                break;

            case 'h':
                print_help(basename(argv[0]), stdout);
                exit(EXIT_SUCCESS);
                break;

            case '?':
                print_bad_flag((char*) &optopt);
                print_help(basename(argv[0]), stderr);
                exit(EXIT_FAILURE);
                break;

            default:
                abort();
        }
    }
}

---

main() looks generally fine. You can now initialize buffer_length = 0 and buffer = NULL for simplicity, since getline() takes care of memory allocation for you.

There was one comment, // If realloc changes the location of memory, we need to know this., which I found puzzling. I don't think it adds any value, so I recommend removing it.