2
\$\begingroup\$

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

Note: the next iteration at A trivial command line utility for trimming whitespace from lines in C - follow-up 3

Now my code looks like:

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

#define HELP_FLAG           "-h"
#define VERSION_FLAG        "-v"
#define FLAG_DESC           "%-5s"
#define INITIAL_BUFFER_SIZE 64
#define FALSE               0
#define TRUE                (~FALSE)

/*******************************************************************************
* 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 int 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 FALSE;
        }

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

        }

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

/*******************************************************************************
* Prints the help message and exits.                                           *
*******************************************************************************/
static void print_help()
{
    printf("Usage: trim [" 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",
           HELP_FLAG,
           VERSION_FLAG);
}

/*******************************************************************************
* Prints the version string.                                                   *
*******************************************************************************/
static void print_version()
{
    printf("trim 1.618\n" \
           "By Rodion \"rodde\" Efremov 09.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)
{
    for (size_t i = 1; i < argc; ++i)
    {
        if (strcmp(argv[i], HELP_FLAG) == 0)
        {
            print_help();
            exit(EXIT_SUCCESS);
        }
        else if (strcmp(argv[i], VERSION_FLAG) == 0)
        {
            print_version();
            exit(EXIT_SUCCESS);
        }
        else if (argv[i][0] == '-')
        {
            print_bad_flag(argv[i]);
            exit(EXIT_FAILURE);
        }
    }
}

/*******************************************************************************
* 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 (size_t 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. No casting types with malloc/realloc
  2. sizeof(char) assumed to be 1
  3. Constants used only once are not #defined
  4. Variables are declared as close to their usage as possible.
  5. The whitespace trimming function implements a state machine doing only one pass over the input string.
  6. The linked list of chars -idea removed. Instead, a buffer is doubled everytime a new line does not fit.
\$\endgroup\$
  • 1
    \$\begingroup\$ Are you aware of <stdbool.h>? \$\endgroup\$ – glampert Apr 9 '15 at 17:53
2
\$\begingroup\$
  • You don't have to define custom true/false constants and boolean types. C99 made <stdbool.h> standard. It will provide you with bool, true and false, which are quite handy for return values and well... booleans!

  • The downside of hardcoding the name trim in the help/info strings is that if the name of the binary is changed/aliased it will still print trim in the help run. If you use argv[0], this issue goes away. Unless of course the hardcoded string is the intended behavior.

  • Consider using a standard argument parsing tool such as getopt or argp_parse. No need to reinvent the wheel when such great tools already exist. See a list here.

  • If I compile this with all warnings enabled on Clang (-Wall, -Wextra) I get the output:

    test.c:82:30: warning: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Wsign-compare]
            for (size_t i = 0; i < *p_buffer_length; ++i)
                               ~ ^ ~~~~~~~~~~~~~~~~
    test.c:159:26: warning: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Wsign-compare]
        for (size_t i = 1; i < argc; ++i)
                           ~ ^ ~~~~
    test.c:198:26: warning: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Wsign-compare]
        for (size_t i = 1; i < argc; ++i)
                           ~ ^ ~~~~
    

    You have a few size_ts comparing against ints. This is harmless 99% of the time, but it wouldn't be a hassle being consistent and using the same type for both variables in this case. p_buffer_length is the length of an array, so it would be more correct as size_t.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.