8
\$\begingroup\$

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] != '\\';
}
\$\endgroup\$
6
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Excellent advice - thanks! I forgot that the increment statement would run even after the last iteration. \$\endgroup\$ – ivan Dec 26 '15 at 16:05
4
\$\begingroup\$

In addition to what was already said:

  • 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.

| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ "instead of the less portable _Bool"? _Bool is a standard type. \$\endgroup\$ – edmz Dec 26 '15 at 14:46
  • \$\begingroup\$ Very good points about code clarity and maintenance efforts. I did find, however, that keeping the index variable names short made it easier to read some lines, but I might revisit that thought. \$\endgroup\$ – ivan Dec 26 '15 at 16:11
  • \$\begingroup\$ @black, I thought that _Bool was an implementation detail of bool, but you're right, it is a standard type in it's own right. Corrected. \$\endgroup\$ – glampert Dec 26 '15 at 21:57
  • \$\begingroup\$ It's indeed the opposite: bool is a typedef for _Bool. \$\endgroup\$ – edmz Dec 26 '15 at 23:48
1
\$\begingroup\$

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);
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Hm, I fail to see why providing a filename would be better. Being able to read from stdin is vastly more useful and flexible (imagine the Unix tool set without that functionality). It's a nice extra but should not replace it. \$\endgroup\$ – ChrisWue Dec 26 '15 at 8:10
  • 1
    \$\begingroup\$ In C, a parameter of type char []should be identical to a parameter of type char *. Under what circumstance would it perform an array copy? \$\endgroup\$ – Erick Wong Dec 26 '15 at 9:03
  • \$\begingroup\$ Some helpful thoughts here, thanks for the response. I didn't want to delve into parsing command line args in this case, and I've been either typing at stdin or running the program with < somesourcefile.c. In get_input I need to initialize the index variable i outside the for loop so it will still be in scope in the following code (where I add the null-byte and return the final index value). Good point about the less-verbose and clearer malloc code. \$\endgroup\$ – ivan Dec 26 '15 at 16:27
1
\$\begingroup\$

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

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ fgets has some caveats: stackoverflow.com/a/16323215/220986 which for text probably doesn't matter but nevertheless should be mentioned. \$\endgroup\$ – ChrisWue Dec 26 '15 at 8:24
  • \$\begingroup\$ I realized the single line comments were out of scope for the original exercise after the fact. It was fun to build out the extra logic to support them, but the very next exercise is complicated enough that I had to stop doing so :) \$\endgroup\$ – ivan Dec 26 '15 at 16:31
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hm, I've been focused on messing around with pointers to get more comfortable with them, but I think you're right about this being a sub-optimal approach for this particular problem. I've posted a different approach here \$\endgroup\$ – ivan Dec 27 '15 at 1:27
1
\$\begingroup\$

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!

| improve this answer | |
\$\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.