5
\$\begingroup\$

Please review my code and tell where I can improve my program.

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

#define BUFFER_SIZE 1024
#define TOKEN_SIZE 32
#define TRUE 1
#define FALSE 0

int isKeyword(const char* token)
{
    const char keywords[32][20]={"auto","break","case","char",
                                "const","continue","default","do","double",
                                "else","enum","extern","float",
                                "for","goto","if","int","long",
                                "register","return","short","signed",
                                "sizeof","static","struct","switch",
                                "typedef","union","unsigned",
                                "void","volatile","while"};
    for(int i=0;i<32;i++)
    {
        if(strcmp(token,keywords[i])==0)
            return TRUE;
    }
    return FALSE;
}

int isIntLiteral(const char* token)
{
    for(int i=0;token[i]!=(char)NULL;i++)
    {
        if(!isdigit(token[i]))
            return FALSE;
    }
    return TRUE;
} 

int isFloatLiteral(const char* token)
{
    if(isdigit(token[0]))
    {
        for(int i=1;token[i]!=(char)NULL;i++)
        {
            if(!isdigit(token[i]) && (token[i])!='.')
                return FALSE;
        }
        return TRUE;
    }
    return FALSE;
} 

int isCharLiteral(const char* token)
{
    if(token[0]=='\'' && strlen(token)==3 && token[2]=='\'') 
        return TRUE;
    else
        return FALSE;
}

int isLiteral(const char* token)
{
    if(isCharLiteral(token)||isIntLiteral(token)||isFloatLiteral(token))
        return TRUE;
    else
        return FALSE;
}

int isIdentifier(const char* token)
{
    if(isalpha(token[0]) || token[0]=='_')
    {
        for(int i=1;token[i]!=(char)NULL;i++)
        {
            if(!isalnum(token[i]))
                return FALSE;
        }
        return TRUE;
    }
    return FALSE;
}

int isOperator(const char* token)
{
    const char operators[41][10] = {".","->","++","--","!","~","(type)","*","&",
                                 "sizeof","*","/","%","+","-","<<",">>","<",
                                 "<=",">",">=","==","!=","&","^","|","&&","||",
                                 "?:","=","+=","-=","*=","/=","%=","&=","^=",
                                 "|=","<<=",">>=",","};

    for(int i=0;i<41;i++)
    {
        if(strcmp(token,operators[i])==0)
            return TRUE;
    }
    return FALSE;
}

FILE* readFile(int param1,char* param2[])
{
    if(param1<2) 
    {
        fprintf(stderr,"No file specifed to read. \n");
        exit(1);
    }

    if(param1>2)
    {
        fprintf(stderr,"Too many arguments for read to perform. \n");
        exit(1);
    }

    FILE *fp = fopen(param2[1],"r");

    if(!fp)
    {
        fprintf(stderr,"File access denied on read. \n");
        exit(1);
    }
    return fp;
}

int main(int argc,char* argv[])
{

    FILE *fp = readFile(argc,argv);

    char *token;
    char string[BUFFER_SIZE];
    const char delimiters[]=" .,;!=\n";

    fgets(string,sizeof string, fp);
    while(string!=(char *)NULL || (strcmp(string,"\n"))==0)
    {
        token = strtok(string,delimiters);

        while(token!=NULL)
        {
            if(isKeyword(token))
                printf("%s is a keyword. \n",token); 
            else if(isOperator(token))
                printf("%s is an operator. \n",token);
            else if(isIdentifier(token))
                printf("%s is an identifier. \n",token);
            else if(isLiteral(token))
                printf("%s is a literal. \n",token);
            else
                printf("Unrecognized token : %s \n",token);

            token = strtok(NULL,delimiters);
        }
        fgets(string,sizeof string, fp);
    }   

    fclose(fp);
    return 0;
}`
\$\endgroup\$
  • \$\begingroup\$ What if i want to extract keyword from a given statement. \$\endgroup\$ – mayor Feb 27 '18 at 11:58
7
\$\begingroup\$

Remove all these literals from the stack and place them in the .rodata/.const section of the executable image:

const char keywords[32][20]={"auto","break","case","char",
                            "const","continue","default","do","double",
                            "else","enum","extern","float",
                            "for","goto","if","int","long",
                            "register","return","short","signed",
                            "sizeof","static","struct","switch",
                            "typedef","union","unsigned",
                            "void","volatile","while"};

One very good way to do that would be to make it an array of pointers:

const char *keywords[]=
{
    "auto","break","case","char",
    "const","continue","default","do","double",
    "else","enum","extern","float",
    "for","goto","if","int","long",
    "register","return","short","signed",
    "sizeof","static","struct","switch",
    "typedef","union","unsigned",
    "void","volatile","while"
};

Similar considerations should be applied to the literal array in the isOperator() function.

Regarding this line:

for(int i=0;i<32;i++)

Do not use 'magic' numbers (other than 0 and 1) in the code as they are a maintenance nightmare. I suggest:

for(size_t i=0; i<(sizeof(keywords)/sizeof(char*)); i++)

Regarding this line:

for(int i=0;token[i]!=(char)NULL;i++)

This will not work quite correctly, as NULL is the size of a pointer and I think the code is looking for '\0', not NULL.

I suggest:

Note, no need to make any comparison as the contents of token[i] is either in the range 0x01...0xFF or 0x00 - and 0x00 will be seen as false.

for(int i=0; token[i];i++)

When #defineing a numeric value, always wrap the value in parens to avoid any possibility of 'text replacement' errors.

Regarding these lines:

fgets(string,sizeof string, fp);
while(string!=(char *)NULL || (strcmp(string,"\n"))==0)
  1. string will NEVER be NULL because string is not a pointer
  2. there is never a need to cast NULL

I suggest:

while( fgets(string,sizeof string, fp) )

To get the actual reason that a call to fopen() fails, change this:

FILE *fp = fopen(param2[1],"r");

if(!fp)
{
    fprintf(stderr,"File access denied on read. \n");
    exit(1);
}

to: (perror() will output the system message for the current value of 'errno')

FILE *fp = NULL;
if( NULL ==( fp = fopen(param2[1],"r") ) )
{ // then fopen failed:
    perror( "fopen for input file failed" );
    exit( EXIT_FAILURE );
}

I suggest that main() check the number of parameters and if not correct, then output a 'USAGE' statement and exit. This would greatly simplify the readfile() function, which is not actually reading a file, but rather just opening a file.

There are plenty of other details that could be improved, but the above should get you started.

\$\endgroup\$
  • \$\begingroup\$ I'd write your "array size" expression as sizeof keywords / sizeof *keywords so it's automatically right regardless of the type of keywords[]. And you're right about casting NULL to char, but not because of the size of the value (the problem is that C allows NULL to be (char*)0, which isn't necessarily represented by zero, and casting to arithmetic types doesn't have to spot NULL and convert it to 0). \$\endgroup\$ – Toby Speight Feb 27 '18 at 14:45
3
\$\begingroup\$

stdbool.h

I suggest using #include <stdbool.h> and the bool type instead of defining TRUE and FALSE yourself.

Literals not recognized by your program

  1. Float literals: 1.0e+19, 1.0f
  2. Int literals: -1, 100L, 0xdead
  3. Char literals: '\n', '\007'

Operators

Currently there are two operators that won't actually be checked correctly:

  1. (type) for casts.
  2. ?: for the ternary operator.

Plus, since you use strtok on your input with these delimiters: " .,;!=\n", you also won't see any . , ! = operators since they will be stripped out by strtok.

Tokenizing

If you have some input such as foo+bar, your program will consider the whole expression as one token. It will then fail to pass the "identifier" test because of the + character. Using strtok to tokenize your input isn't sufficient for the C language. However, if you are assuming that your input has spaces around everything, then it will work.

\$\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.