4
\$\begingroup\$

This is my solution for exercise 1-23 from the book "The C programming language".

Exercise 1-23: Write a program that removes all comments from a C program.

Key idea:

  • if the start of a quoted string is detected, then I call a function that prints the entire string. handled '"', '\"'
  • else if a start of a single line comment is detected, I use a second function to skip it.
  • else if a start of a multiple line comment is found, I used a third function to skip it.
  • else just print the char

I made an effort to make the code readable. I'd like your feedback related to all aspects of the solution. Be harsh, but in a constructive way.

#include <stdio.h>
#include <stdbool.h>

#define DOUBLE_QUOTE_CHAR '"'
#define BASK_SLASH '\\'
#define NEW_LINE_CHAR '\n'
#define FORWARD_SLASH '/'
#define ASTERISK '*'
#define SINGLE_QUOTE_CHAR '\''

void print_quoted_string();     /* called when a quote char that indicate begining of string is encountered. print the quoated string then stop */ 
void skip_single_line_comment(); /* called when we see // that indicate start of a single-line comment . skip that comment */ 
void skip_multiple_line_comment();  

int main(void)
{
    char cur_char; 
    bool prev_forward_slash = false;    // to handle //, /*
    bool prev_single_quote = false;     // to handle '"' 
    bool prev_back_slash = false;       // to handle '\"' 

    while ( (cur_char = getchar()) != EOF) {
        if (cur_char == DOUBLE_QUOTE_CHAR && !prev_single_quote && !prev_back_slash) // to exclude '"' and '\"'. Are there any other casse ? 
            print_quoted_string(); 
        else if (prev_forward_slash && cur_char == FORWARD_SLASH) { // this // can not be inside a string. because the first if statement guarentee it.
            skip_single_line_comment();
            prev_forward_slash = false; 
        } 
        else if (prev_forward_slash && cur_char == ASTERISK ) {
            skip_multiple_line_comment(); 
            prev_forward_slash = false; 
        }
        else {
            if (prev_forward_slash) 
                putchar(FORWARD_SLASH);  // since we don't print a forward slash until we read the character after it. 

            if (cur_char == FORWARD_SLASH)    // we should not print this forward slash now since it may be followed by * 
                prev_forward_slash = true;    
            else if (cur_char == SINGLE_QUOTE_CHAR) {
                prev_single_quote = true; 
                putchar(cur_char);
            }
            else if (cur_char == BASK_SLASH) {
                prev_back_slash = true;
                putchar(cur_char);
            } 
            else {
                prev_single_quote = prev_forward_slash =  prev_back_slash = false;
                putchar(cur_char); 
            }
        }
    }

    return 0; 
}


void print_quoted_string()
{
    putchar(DOUBLE_QUOTE_CHAR);   /* print the start quote of the string */ 

    bool prev_char_is_escape_char = false;  /* used to handle the escape sequences issues. for example:  "\\" , "\"", "\\\\\"" */ 
    char cur_char;
    while (1) {
        cur_char = getchar(); 
        
        if (cur_char == DOUBLE_QUOTE_CHAR && !prev_char_is_escape_char)    /* this quote char is the end quote of the string */ 
            break; 
        
        putchar(cur_char);

        /* current char is an escape char iff it's a back slash and not proceeded by escape char */ 
        prev_char_is_escape_char = (cur_char == BASK_SLASH && !prev_char_is_escape_char); 
    }

    putchar(DOUBLE_QUOTE_CHAR);    /* print the end quote of the string */
}

void skip_single_line_comment()
{
    while (getchar() != NEW_LINE_CHAR); 
    putchar(NEW_LINE_CHAR);
}

void skip_multiple_line_comment()
{
    char prev_char = '/'; 
    char cur_char = '*'; 

    while (1) {
        char cur_char = getchar(); 

        if (prev_char == '*' && cur_char == '/') {
            putchar(NEW_LINE_CHAR); 
            return; 
        }

        prev_char = cur_char; 
    }
}

\$\endgroup\$
2
\$\begingroup\$

Few problems:

  • If a quoted string is not terminated, print_quoted_string becomes an infinite loop. Granted, such input is malformed, but your program shall fail gracefully.

  • Pay attention to a backslash-newlines:

    // This \
      is a comment.
    
    /\
    / This is also a comment
    
    /\
    * Even this is a comment */
    

I strongly recommend to move the single quote processing into a function. This will greatly reduce the number of state variables. Also, keep in mind that the stray backslash has no meaning. Usually it is a syntax error; in any case it does not escape anything.

\$\endgroup\$
6
\$\begingroup\$

First a classic bug: char cur_char; ... (cur_char = getchar()) != EOF. The variable must be int not char or you can't compare it with EOF. Yeah it's really stupid that getchar gets an int, not a char, but that's how it is.


I know this is just a simple program and performance, maintainability etc isn't import. If it was a real production quality program though, it would preferably be written differently. For the sake of learning, lets pretend it is:

Then overall, you could be checking against a look-up table rather than by using a complex series of if-else if. They are kind of hard to read, you get the various different behavior upon finding certain comment characters scattered over various nested if-else if. Also the compiler is less likely to translate the if-else if to some table look-up, more likely this would generate a bunch of branches which are very bad for loop performance.

A look-up table followed by a centralized "take action depending on result" code like for example a switch would improve execution speed and readability/maintainability both.

The simplest form of such a table lookup would be to strchr("\"\\\n/*\'", input) then take different actions based on if strchr returned NULL or not.

So rather than defining all comment characters with macros, you'd rather have a typedef enum { DOUBLE_QUOTE_CHAR, BACK_SLASH, ... NO_COMMENT } comment_t; etc corresponding to the index passed to the string literal used by strchr. Then you can do:

  const char  comment_characters[] = "\"\\\n/*\'";
  const char* comment_found = strchr(comment_characters, input)
  comment_t   comm;

  if(comment_found)
    comm = (comment_t) (comment_found - comment_characters); // pointer diff arithmetic
  else // strchr returned NULL
    comm = NO_COMMENT;

  switch(comm)
  {
     case DOUBLE_QUOTE_CHAR:  /* do double quote stuff */  break;
     case BACK_SLASH:         /* do backslash stuff */     break;

     default:                 /* NO_COMMENT etc, do nothing */
  }

You can even take readability/maintainability a bit further almost to extremes by doing this instead:

  const char comment_characters [N] = // where N is some size "large enough"
  {
    [DOUBLE_QUOTE_CHAR] = '\"',
    [BACK_SLASH] = '\\',
    [N-1] = '\0',                     // strictly speaking not necessary but being explicit is nice
  };

This guarantees integrity between the string and the enum indices.

\$\endgroup\$
4
  • \$\begingroup\$ I write this solution based only on what I have learned from chapter 1 of the book. So, I didn't know any of what you said. which makes your reply very useful. thank you so much. \$\endgroup\$
    – mark coder
    Feb 17 at 16:18
  • 1
    \$\begingroup\$ getchar() returning an int is very much not stupid. If it returned a char, the EOF value would need to also fit in a char, which would mean it would need to have the value of some valid actual character. That would mean that you'd need to have some other way to figure out if the EOF return value was an actual end-of-file, or the character. E.g. with strtol(), you need to check errno. But since it's not modified if there is no error, it means you need to set errno = 0 before each call, if you want to detect errors. That's worse than returning int. \$\endgroup\$
    – ilkkachu
    Feb 17 at 20:29
  • \$\begingroup\$ @ilkkachu There's no rationale here. Back in the days when all of this was designed at a whim, 7 bit token tables were used, so they could simply have picked any value with MSB set. Such as 0xFF. But no, surely it must be 0xFFFF...! Also god forbid if they designed the function as normally done in professional C, separating data from errors. char getchar (err_t* result);, that would have been to logical and readable. But no, getchar must not get a char! This API is just horrible bad, always been, no excuses, no rationales. Like most of stdio.h, likely the worst library ever designed. \$\endgroup\$
    – Lundin
    Feb 18 at 7:13
  • \$\begingroup\$ @Lundin, I was thinking more like int getchar (char *c); where the return value is the ok/error code, and the character read goes through the pointer. That way it would match most other functions. The caller has to use both anyway, so it's not like it would be in any way easier if the return value was the char. But yeah, I'm not too familiar with the history there, so consider that a post-facto rationalization if you want. Regardless, as long as it only returns one value, and all char values are valid characters, the retval has to be bigger than a char, regardless of what size they are. \$\endgroup\$
    – ilkkachu
    Feb 18 at 9:49

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.