6
\$\begingroup\$

Following is my code for implementing basic symbol table in C. Please review my code and tell where I can improve my program.

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

#define BUFFER_SIZE 1024
#define TOKEN_SIZE 32

typedef struct symbol
{
    char* identifierName;
    char* datatype;
    int offset;
    struct symbol* next;
}symbol;

void insert(symbol** headRef,char* identifier,char* type)
{
    symbol* newnode = (symbol*) malloc(sizeof(symbol));
    static symbol* prevnode;
    newnode->identifierName = (char*) malloc((strlen(identifier)+1)*sizeof(char));
    newnode->datatype = (char*) malloc((strlen(type)+1)*sizeof(char));

    if(*headRef==NULL) 
    {
        newnode->offset=0;
        prevnode = newnode;
    }

    strcpy(newnode->identifierName,identifier);
    strcpy(newnode->datatype,type);

    if(strcmp(newnode->datatype,"int") == 0) 
        (newnode->offset) = (prevnode->offset) + sizeof(int);
    else if(strcmp("float",newnode->datatype) == 0)
        (newnode->offset) = (prevnode->offset) + sizeof(float);
    else if(strcmp("char",newnode->datatype) == 0)
        (newnode->offset) = (prevnode->offset) + sizeof(char);
    else if(strcmp("double",newnode->datatype) == 0)
        (newnode->offset) = (prevnode->offset) + sizeof(double);

    prevnode = newnode;
    newnode->next = (*headRef);
    (*headRef) = newnode;
}

void display(symbol* node)
{
    while(node!=NULL)
    {
        printf("%s %s %d \n",node->identifierName,node->datatype,node->offset);
        node = node->next;
    }
}

bool 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;
}

bool isDatatype(const char* token)
{
    const char *datatype[]={"char","double","float","int"};
    for(size_t i=0;i<(sizeof(datatype)/sizeof(char*));i++)
    {
        if(strcmp(token,datatype[i])==0)
            return true;
    }
    return false;
}

void isDeclaration(char* statement,symbol** head)
{
    char *delimiters = " ,;\t";
    char *token = strtok(statement,delimiters);
    if(!isDatatype(token))
        return;
    char *datatype = token;
    while((token=strtok(NULL,delimiters)))
        if(isIdentifier(token))
            insert(head,token,datatype);    
}

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

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

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

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


int main(int argc, char *argv[])
{   
    FILE *fp = openFile(argc,argv);

    char string[BUFFER_SIZE];
    symbol* head = NULL;

    while(fgets(string,sizeof string, fp))
        isDeclaration(string,&head);

    display(head);
    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ regarding this line: 'symbol* newnode = (symbol*) malloc(sizeof(symbol));' 1) in C, do not cast the returned value from malloc and family of functions. This is because the returned value is a void*, so can be assigned to any other pointer and maintenance becomes much easier as there is no need to fix the casting. 2) always check (!=NULL) the returned value to assure the operation was successful. 3) the expression 'sizeof(char)' is always 1, so adds nothing to the value passed to malloc() \$\endgroup\$ – user3629249 Aug 23 '15 at 4:25
  • 1
    \$\begingroup\$ regarding this line: 'const char *datatype[]={"char","double","float","int"};' to avoid having the execution re-create this every time the function is called, add a 'static' modifier. I.E. 'static const char *datatype[]={"char","double","float","int"};' \$\endgroup\$ – user3629249 Aug 23 '15 at 4:35
  • \$\begingroup\$ regarding: 'char *token = strtok(statement,delimiters)' on every call to strtok(), always check (!=NULL) the returned value to assure the operation was successful. A failure of the call to strtok() will result in 'isIdentifier()' trying to read from address 0. This is undefined behaviour and can lead to a seg fault event \$\endgroup\$ – user3629249 Aug 23 '15 at 4:39
  • 1
    \$\begingroup\$ in general, when checking the parameters (as in the openfile() function) When the code finds a problem with the command line parameters, it should display a usage statement, so the user is informed of exactly what they should have done. \$\endgroup\$ – user3629249 Aug 23 '15 at 4:43
  • 7
    \$\begingroup\$ @user3629249 Why don't you put all this advice into an answer? \$\endgroup\$ – JS1 Aug 23 '15 at 5:39
2
\$\begingroup\$

I transcribed the comments from @user3629249 into this community wiki answer:

  • Regarding this line:

    symbol* newnode = (symbol*) malloc(sizeof(symbol));
    
    1. In C, do not cast the returned value from malloc and family of functions. This is because the returned value is a void*, so can be assigned to any other pointer and maintenance becomes much easier as there is no need to fix the casting.
    2. Always check (!=NULL) the returned value to assure the operation was successful.
    3. The expression sizeof(char) is always 1, so adds nothing to the value passed to malloc()
  • Regarding this line:

    const char *datatype[]={"char","double","float","int"};
    

    To avoid having the execution re-create this every time the function is called, add a static modifier. I.E. static const char *datatype[]={"char","double","float","int"};

  • Regarding:

    char *token = strtok(statement,delimiters)
    

    On every call to strtok(), always check (!=NULL) the returned value to assure the operation was successful. A failure of the call to strtok() will result in isIdentifier() trying to read from address 0. This is undefined behaviour and can lead to a seg fault event.

  • In general, when checking the parameters (as in the openfile() function) When the code finds a problem with the command line parameters, it should display a usage statement, so the user is informed of exactly what they should have done.

  • When a system function returns an error indication, such as the returned value from fopen(), the next line of code should be perror(); so the related system error message gets output to stderr.

  • When #defineing numeric values, always wrap the numeric value with parens so there are no 'text replacement' errors.

  • Due to each new token being inserted at the beginning of the linked list, when the list is displayed, the tokens will be displayed in the reverse order. Is this what you want?

  • A symbol table needs to contain info on any modifiers being applied to a symbol. including when a modifier is a function name, a returned value, a returned type, etc etc linked values, such as struct's enum's, union's, etc need links between the type, the name, and the associated fields with their types, names, and order. The current code handles none of that.

  • The isDeclaration() function will treat parens, braces, and other similar items incorrectly. The parsing needs to keep a 'state' history, so as to know how to treat the next token. (all part of lexical and syntax analysis) The posted code does not properly handle strings in double quotes and single quotes. The posted code does not handle a '\' at the end of an input line.

  • The max token length is set to 31 characters+string delimiter. what is this code was reading a C source file? while only the first 32 characters may be significant, an identifier can be much longer. the code will overrun the token buffer in such an instance, resulting in undefined behaviour, leading to a seg fault event.

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