K&R 1-23 Remove all C comments

Question

From K&R exercise 1-23,

Write a program to remove all comments from a C program. Don't forget to handle quoted strings and character constants properly. C comments don't nest.

My solution,

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

#define MAXLEN  10000   // max input length

int      get_input(char input[], int maxlen);
char * no_comments(char input[], int len);

_Bool comment_start(char input[], int idx);
_Bool   comment_end(char input[], int idx, char *comments, int cdx);

_Bool quote_start(char input[], int idx);
_Bool   quote_end(char input[], int idx);

int main(void)
{
    char input[MAXLEN];
    int len = get_input(input, MAXLEN);
    char *scrubbed = no_comments(input, len);

    printf("%s", scrubbed);
    free(scrubbed);
}

int get_input(char input[], int maxlen)
{
    int i;
    char c;

    for (i = 0; i < maxlen-1 && (c = getchar()) != EOF; i++) {
        input[i] = c;
    }
    input[++i] = '\0';

    return i;
}

char * no_comments(char input[], int len)
{
    char *scrubbed = (char *) malloc((len+1) * sizeof(char));
    int sdx = 0;    // index

    char *comments = (char *) malloc((len+1) * sizeof(char));
    comments[0] = '\0';
    int cdx = 0;    // index

    _Bool quoted = 0;

    for (int idx = 0; idx <= len; idx++) {
        if (strlen(comments) == 0) { // outside comment

            if (!quoted && comment_start(input, idx)) {
                comments[cdx++] = input[idx];
            } else {
                scrubbed[sdx++] = input[idx];
            }

            if (quoted && quote_end(input, idx)) {
                quoted = 0;
            } else if (!quoted && quote_start(input, idx)) {
                quoted = 1;
            }

        } else { // inside comment

            if (comment_end(input, idx, comments, cdx)) {
                comments[0] = '\0'; // exit comment
                cdx = 0;
            } else {
                comments[cdx++] = input[idx];
            }
        }
    }
    free(comments);

    return scrubbed;
}

_Bool comment_start(char input[], int idx)
{
    char nxt = input[idx+1];

    return input[idx] == '/' && (nxt == '*' || nxt == '/');
}

_Bool comment_end(char input[], int idx, char *comments, int cdx)
{
    char prev = comments[cdx-1];
    char cur  = input[idx];
    char nxt  = input[idx+1];

    _Bool new_style = (cdx == 1 && cur == '/') || (cdx > 1 && comments[1] == '/');
    _Bool old_style = (cdx == 1 && cur == '*') || (cdx > 1 && comments[1] == '*');

    return (new_style && nxt == '\n') || (old_style && cdx > 2 && prev == '*' && cur == '/');
}

_Bool quote_start(char input[], int idx)
{
    return input[idx] == '"' && input[idx-1] != '\'';
}

_Bool quote_end(char input[], int idx)
{
    return input[idx] == '"' && input[idx-1] != '\\';
}

Answer 1

I recommend using Valgrind as the first step of the self-code review. Here it only reports one error when compiled with gcc on Linux:

 $ gcc -O2 main.c -o main -std=c99 -Wall -Wextra  -g
 $ valgrind  --track-origins=yes ./main
 (...)
 ==24100== Conditional jump or move depends on uninitialised value(s)
==24100==    at 0x4009B4: comment_start (main.c:85)
==24100==    by 0x4009B4: no_comments (main.c:54)
==24100==    by 0x40062E: main (main.c:21)
==24100==  Uninitialised value was created by a stack allocation
==24100==    at 0x400620: main (main.c:20)
==24100==
==24100== Conditional jump or move depends on uninitialised value(s)
==24100==    at 0x4009CA: quote_start (main.c:102)
==24100==    by 0x4009CA: no_comments (main.c:62)
==24100==    by 0x40062E: main (main.c:21)
==24100==  Uninitialised value was created by a stack allocation
==24100==    at 0x400620: main (main.c:20)
==24100==
==24100== Conditional jump or move depends on uninitialised value(s)
==24100==    at 0x518C1B2: vfprintf (in /lib64/libc-2.21.so)
==24100==    by 0x51928E8: printf (in /lib64/libc-2.21.so)
==24100==    by 0x400640: main (main.c:23)
==24100==  Uninitialised value was created by a stack allocation
==24100==    at 0x400620: main (main.c:20)
=

The problem lies inside get_input(), exactly in this line:

input[++i] = '\0';

Add printing value of i after each iteration and after the pre-incrementation. You will see something like this:

i == 0
i == 1
i == 2
i == 3
i == 4
i == 5
i == 6
i == 7
i == 8
i == 9
i == 11

Position 10 is not set. However, later on in no_comments() you have a loop:

for (int idx = 0; idx <= len; idx++) {
    if (strlen(comments) == 0) { // outside comment

        if (!quoted && comment_start(input, idx)) {

Input array and index are passed to comment_start() and read there:

return input[idx] == '/' && (nxt == '*' || nxt == '/');

At some point you will read an element of input array that has not been initialized. It's an undefined behavior in C and it's quite dangerous.

Answer 2

In addition to what was already said:

• c99 standardized the bool type, declared in the <stdbool.h> header file. It also defines true/false constants for you. You should use it instead of the less portable and certainly not pretty _Bool.

• #define MAXLEN 10000 // max input length If you instead call it MAX_INPUT_LENGTH, no need for the comment and you gain on overall code clarity.

• Function prototypes could be avoided, all you have to do is move main() to the end of the file. Having both the prototype and the definition increases maintenance effort, if one changes, both must be updated. Avoid that if you can.

• sdx, cdx, ndx are not very good names. Spelling out comment_index and such is better. There's no need to create unclear abbreviations for the sake of avoiding a few keystrokes.

Answer 3

In addition to what was said previously, your method of obtaining input is extremely tedious for the user. As it stands your program reads in a single character from stdin each time. It is better to allow the user to call your program with an argument containing the name of a file containing C source code:

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

#define MAX_SIZE 10000

/* Obtain file size 
 *
 * @param[in] -- a valid pointer to a stream
 *
 * @return -- size of the file in bytes
 */
size_t fsize(FILE *file)
{
    if (!file) { return 0; }

    size_t pos = ftell(file);

    fseek(file,  0, SEEK_END);
    size_t size = ftell(file);

    /* Restore original position of file */
    fseek(file, pos, SEEK_SET);

    return size;
}

char* get_input(const char *file_name, size_t max_size, size_t *input_size)
{
    FILE *file = fopen(file_name, "r");
    if (!file) {
        fprintf(stderr, "Couldn't open %s\n", file_name);
        return NULL;
    }

    size_t file_size = fsize(file);

    if (file_size > max_size) {
        fprintf(stderr, "File too big!\n");
        fclose(file);
        return NULL; 
    }

    char *buffer = malloc(file_size * sizeof(*buffer));
    if (!buffer) {
        fprintf(stderr, "Not enough memory to contain file\n");
        fclose(file);
        return NULL;
    }

    size_t result = fread(buffer, sizeof(*buffer), file_size, file);    
    if (result != file_size) {
        fprintf(stderr, "Read error!\n");
        fclose(file);
        free(buffer);
        return NULL;
    }

    fclose(file);   
    *input_size = file_size;
    return buffer;
}


int main(int argc, char **argv)
{
    if (argc != 2) {
        fprintf(stderr, "Invalid args\n");
        return 1;
    }

    size_t input_size = 0;
    char *input = get_input(argv[1], MAX_SIZE, &input_size);

    if (!input) {
        return 1;
    }

    /* Finally got clean input */
    no_comments(input, input_size);
    free(input);
}

It looks like you're also using C99 or later here. There is no need to initialize your index variable outside your for loop - especially since you don't do that in all your instances of for loops:

int i;
char c;

for (i = 0; i < maxlen-1 && (c = getchar()) != EOF; i++) {
    input[i] = c;
}

Assuming you're not going to port this code to C++ anytime soon, this is too verbose of a statement and your comments are unnecessary.

char *scrubbed = (char *) malloc((len+1) * sizeof(char));
int sdx = 0;    // index

char *comments = (char *) malloc((len+1) * sizeof(char));
int cdx = 0;    // index

You can make this more succinct (and therefore clearer) like so:

char *scrubbed = malloc(len + 1, sizeof(*scrubbed));
int scrubbed_index = 0; // <-- this is more clear than a short variable
                        // variable name with a comment next to it

char *comment  = malloc(len + 1, sizeof(*comment));
int comment_index = 0;

On the other hand, idx is too verbose; simply using good 'ol i would suffice.

You also forget to null-terminate your scrubbed array before returning it from no_comments().

Finally, it looks like all your functions accept a char[] which is inefficient since the entire array will be copied into each of these functions. It would be much more efficient to pass in the array via a pointer for these functions.

int      get_input(char *input, int maxlen);
char * no_comments(char *input, int len);

_Bool comment_start(char *input, int idx);
_Bool   comment_end(char *input, int idx, char *comments, int cdx);

_Bool quote_start(char *input, int idx);
_Bool   quote_end(char *input, int idx);

Answer 4

Too modern

In the version of C described in K&R, there are no single line comments. Therefore, there is no need to worry about them in your code.

Yes, your code is good to include these; there's nothing wrong with taking an extra step. However, since you are following an exercise, I thought I'd bring it up.

---

Over-complicating input

TamerCpp is right in his recommendation for how to handle input; you should definitely go with that.

However, as a review for what you have present, you are seriously over-complicating how you receive input. There is already a function for exactly what you are doing: fgets.

In fact, with this function, you don't even need your get_input function anymore. Instead, you would simply write:

fgets(input, maxlen, stdin);

---

Answer 5

I have not read through your code carefully, because you appear to have made three closely-related, fundamental design errors.

It is possible to write a program that does this job without using malloc, and without imposing any upper limit on the length of the input or the length of an individual line. It can be done using less than 16 bytes of scratch variables, total.

Your homework is to figure out how to do that.

Answer 6

I'm a little late, but here are some additional comments to your code:

• As commented upon in your related question, you do have some potential issues related to quoting backslashes and quotes. You can't just check for the last quote character, you need to check if that is a quoted character or not as well.
• No need to store the comment, when you are throwing it away anyway. All code related to comments and cdx can be removed.

• Realloc the result before returning – At the start of your code you malloc enough memory for the entire string, which is needed as you don't need if there are actually any comments to remove. But when returning you know the exact size, and I think it would be better to reallocate the memory needed for storing the result (and free up the extra memory used in your function).

  A realloc would also give some meaning as to why one would want to remove the comments, as later code could possibly use the now minimized code with a lower memory footprint.

• Consider switching around boolean logic into direct statements – In some cases doing stuff like if (condition) { result = true; } else { result = false; } could be changed into the simpler: result = condition;. This could simplify some of your logic, and avoid the described anti-pattern.

• Introduce comments and/or quote loops – Consider adding a while loop fast forwarding when you detect the start of a comment or quote. When the start is detected, you are not interested in anything else until the quote or comment has ended.

Hope this gives you some new thoughts in addition to the other answers already given!