Program that removes comments from C and C++ programs

Question

I have downloaded the latest version of the Linux kernel and I want to perform thorough analysis on it. I want to start by eliminating comments from all the source and header files that belong to it. I am using a code I wrote sometime ago and I want to use it recursively. In the past I tested it on my code and it worked fine, but since I want to use it on a wide scale, I want to know if the code is consistent and successful in all cases.

Can you help me determine if it has bugs that do not deal with removing comments and create mess in certain cases?

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

int RemoveComments(const char *filename, const char *Output);


int main(void)
{
    RemoveComments("file.cpp", "output.cpp");
    return 0;
}

int RemoveComments(const char *filename, const char *Output)
{
#define NOT_COMMENT (!DOUBLESLASH_Comment && !ASTERISK_SLASH_Comment)

    clock_t t1 = clock();
    FILE *input, *output;
    if (fopen_s(&input, filename, "r"))
    {
        perror(filename);
        exit(1);
    }
    if (fopen_s(&output, Output, "w"))
    {
        perror(Output);
        exit(1);
    }
    int c, d;

    bool DOUBLESLASH_Comment = 0, ASTERISK_SLASH_Comment = 0, flag = 0;

    int s_QUOTED = 0, d_QUOTED = 0;
    while ((c = getc(input)) != EOF)
    {
        switch (c)
        {
        case '\\':
        {
            if (NOT_COMMENT)
            {
                if (flag == 1)
                    flag = 0;
                else
                    flag = 1;
            }
        }break;
        case '\'':
        {
            if (NOT_COMMENT && !d_QUOTED)
            {
                if (!flag)
                {
                    s_QUOTED++;
                }
            }
        }break;
        case '"':
        {
            if (NOT_COMMENT && !flag)
            {
                if (!s_QUOTED)
                {
                    d_QUOTED++;
                }
            }
        }break;
        case '/':
        {
            if (NOT_COMMENT && !d_QUOTED)
            {
                if ((d = getc(input)) == '*')
                {
                    ASTERISK_SLASH_Comment = 1;
                }
                else if (d == '/')
                {
                    DOUBLESLASH_Comment = 1;
                }
                else
                {
                    if (d != EOF)
                    {
                        ungetc(d, input);
                    }
                }
            }
        }break;
        case '*':
        {
            if (ASTERISK_SLASH_Comment)
            {
                if ((d = getc(input)) == '/')
                {
                    if ((c = getc(input)) == EOF)
                        return 0;
                    ASTERISK_SLASH_Comment = 0;
                }
                else
                {
                    if (d != EOF)
                    {
                        ungetc(d, input);
                    }
                }
            }
        }break;
        case '\n':
        {
            if (DOUBLESLASH_Comment)
            {
                DOUBLESLASH_Comment = 0;
            }
        }break;
        }
        if (NOT_COMMENT && c != '\\') flag = 0;
        if (d_QUOTED == 2) d_QUOTED = 0;
        if (s_QUOTED == 2) s_QUOTED = 0;
        if (NOT_COMMENT)
        {
            putc(c, output);
        }
    }
    fclose(input);
    fclose(output);
    clock_t t2 = clock();
    double elapsed = (double)(t2 - t1) / CLOCKS_PER_SEC;
    printf("time elapsed : %.4f\n", elapsed);
    return 0;
}

Answer 1

I see some things that may help you improve your code.

Use all required #includes

The program uses the bool type and assert() but doesn't have the associated required includes. Fix that by adding this:

#include <stdbool.h>
#include <assert.h>

Use required #defines

As noted in http://en.cppreference.com/w/c/io/fopen :

As all bounds-checked functions, fopen_s is only guaranteed to be available if STDC_LIB_EXT1 is defined by the implementation and if the user defines STDC_WANT_LIB_EXT1 to the integer constant 1 before including .

Because it's an optional part of the C11 standard, it's also non-portable in a strict sense. For portability, you may wish instead to use the standard fopen and do your own error checking instead.

Think of the user

There are a few user-oriented things I think could be improved. First, it would probably be better to have the filenames passed in via the command line rather than being hardcoded, and second, it would probably be good to allow the user to specify whether the timing is displayed or not.

Fix the bug

There is a problem with the parser at the moment. If we have lines like these:

/* comment one *///  comment two 

the code should remove both comments, but at the moment, it only removes the first one.

Use better variable names

Variables named s_QUOTED and d_QUOTED are OK because their names suggest their meanings, but c, d and flag are not good names.

Consider using named states

It would likely make the code easier to write correctly with named states. One convenient way to do that is to use an enum.

enum { INCODE, INQUOTE, INDBLQUOTE, INCOMMENT, GOTSLASH } state = INCODE;

Use one line per declaration

The code currently includes lines like this:

bool DOUBLESLASH_Comment = 0, ASTERISK_SLASH_Comment = 0, flag = 0;

It makes things a little bit harder to read than if you put them on separate lines like this:

bool DOUBLESLASH_Comment = 0;
bool ASTERISK_SLASH_Comment = 0; 
bool flag = 0;

Additionally, while it's not strictly incorrect, it helps the reader if the code uses false and true rather than numeric values for boolean variables.

Simplify your code

The code currently includes this compound if statement:

if (flag == 1)
    flag = 0;
else
    flag = 1;

This can be simplified:

flag = !flag;

Use braces appropriately

In a number of cases, as with the if statement mentioned above, curly braces ({}) were not used, but they are used for each case statement. In fact, they're not needed in either case, but I'd recommend using braces for constructs such as for, while and if and omitting them for case.

Return something useful from functions

The RemoveComments function is currently written to return int but the only value it ever returns is 0. I'd recommend either returning something useful, such as, for instance a number of comments removed, or to change it to a void function.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

Answer 2

If you want to be pedantic about the C you parse, a newline can actually be escaped with a backslash, continuing the line. Even continuing a // comment, or escaping a newline in the middle of a /*. Not that I'd recommend writing code like that, but you might want to be aware of it.

#define NOT_COMMENT (!DOUBLESLASH_Comment && !ASTERISK_SLASH_Comment)

Frankly, I think this looks horrible. A macro that acts like a function should look like a function, not a constant. One of the reasons you're doing this, is that your variable names are too long, so you resort to tricks to keep the line length under control. There's a bit of a relationship between the length of a variable name, and the scope within which it is used: variables used inside a function or a single loop can have shorter names than globals, since the reader doesn't need to remember them for very long.

Helper functions like this are ok, but they need to look like functions to not confuse the reader, and also because you might want to implement them as inline functions. So, make a variable state, and a function like in_comment(state). Make state a bitfield or a struct if you need. It might also be good to make test functions like this act in the positive sense, as in in_comment(), instead of not_in_comment(), just because ! not_in_comment() may be a bit confusing if it's ever needed.

Though in this case is good to note that you don't need two separate variables for the two kinds of comments, since only one type can be active at a time. This shows later in your code, as setting DOUBLESLASH_Comment is protected with an if (NOT_COMMENT), etc.

if (fopen_s(&input, filename, "r")) {
    perror(filename);
    exit(1);

Don't exit() from inside a function, just return an error code and leave the decision on how to deal with the error to the main program. (Though here, you'd need to close the first file too if the second open fails.) In the same vein, I'd leave the timekeeping to the main program, too.

case '\\': {
    if (NOT_COMMENT) {
        if (flag == 1)
            flag = 0;

While I don't think variable names need to be especially long, flag is much too generic to be of any use (at least without a comment). escape perhaps? (Should the backslash also be checked only inside quotes? I'm not sure.)

if (NOT_COMMENT && !d_QUOTED) {
    if (!flag)

This could be just if (NOT_COMMENT && !d_QUOTED && !flag). Split over two lines if you like. Added confusion since the next almost identical case has the !s_QUOTED and !flag tests in the opposite order.

Though again, you can only be inside a comment or a quoted string, not both, so one variable for those should be enough.

case '/': {
    if (NOT_COMMENT && !d_QUOTED)

Should this have && !s_QUOTED too?

if ((d = getc(input)) == '*') ...
else if (d != EOF) {
        ungetc(d, input);

Since the interesting cases need looking ahead to the next character, you might consider pulling the next character in unconditionally at the start of the loop, so you don't need to duplicate the ungetc calls in different branches of the function.

case '"':  ... d_QUOTED++;
...
if (d_QUOTED == 2) d_QUOTED = 0;

I find this logically odd. There's no way to nest quotes, so instead of a incrementing it as a counter, I'd just flip the value when encountering a quote, somewhat like you did with flag. So, d_QUOTED = !d_QUOTED.

Answer 3

MSalters is right:

You do know that GCC can already do this? The C preprocessor historically was a separate component, and GCC still can emit the result of preprocessing.

So let's save the comment infested example program from cppreference.com as comments.c:

/* C-style comments can contain
multiple lines */
/* or just one */

// C++-style comments can comment one line

// or, they can
// be strung together

int main()
{
  // The below code won't be run
  // return 1;

  // The below code will be run
  return 0;
}

Then, according to the SO question Can gcc output C code after preprocessing?, invoke cpp with the -E argument like so in terminal:

$ cpp -E comments.c
# 1 "comments.c"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 1 "<command-line>" 2
# 1 "comments.c"
# 10 "comments.c"
int main()
{




  return 0;
}

For prettiness, according to the SO question How to remove lines added by default by the C preprocessor to the top of the output?, add the -P argument:

$ cpp -E -P comments.c 
int main()
{
  return 0;
}

The cool thing is that the preprocessor is made to be specification compliant. Should they change something about the comments or if there's some wonky way to place a comment in a templated macro contraption inside an XML String literal that's still according to spec but breaks your regex (or whatever you are using), you're screwed. Think of the preprocessor as an executable version of the specification.

Answer 4

Is your code bugged in the following case?

printf(" /*This is not a comment but removing me changes program behaviour*/");

This one more weird

if( match("/*")){
    doMagicStuff();
}
else
    match("*/");

You can incurr the risk of it becoming reduced to

 if( match(""));  //code that compile fine and have unpredictable side effects

You have to write a complete parser (a simple one, but too complex for becoming a single answer) to avoid these issues.

You can know if you have done everything correctly luckily:

• Compile the kernel ==> binaries A
• Strip comments
• Compile the kernel ==> binaries B
• If binaries A and B are not the same, then your comment remover has some bug.

Answer 5

Well done for handling this using a state machine rather than a regular expression.

However, as others have said, you can't get there from here. Your code will always fail in some cases unless you write a full c lexer, so your best approach is to just use a c compiler to do the job for you:

gcc -fpreprocessed -dD -E file.cpp

That said, why can't you get there from here?

Because in order to recognize just the scope of double-slash comments and multiline comments, your lexer must also understand backslash-continuations, trigraphs like "??/", different file encodings, and doublequotes. And since it must handle quotes, so it must handle hex, octal, unicode and backslash character escapes. And then you need to handle anything new that gets added in at any point, like "raw string literals" (a C++11 thing like heredocs).

Your code does not currently handle these things. Making it handle these things by writing a full C/C++ lexer would be a good learning experience, but... perhaps more interesting challenges lie elsewhere?