Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This program converts Brainfuck source code to C and compiles it with gcc. It runs very well (that was my first time I played Lost Kingdom), however, the code is quite long because some parts are repeated.

Is there a way to reduce the code? What about the "optimization" method? Can this simple optimization be improved?

PS: This code is for Linux, but can be changed for Windows.

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

int main(int argv, char* argc[]){
    //help message
    if(argv==1){
        printf("\n");
        printf("How to use : %s filename [-o output|-c|-d|-r]\n",argc[0]);
        printf("-o output : Set output file name\n-c : Do not compile\n-d : Do not delete C source file\n");
        printf("INFO : You SHOULD type 'export MALLOC_CHECK_=0' manually to remove warning.\n");
        printf("\n");
        return 0;
    }
    printf("INFO : You may type 'export MALLOC_CHECK_=0' manually to remove warning.\n");
    int i;
    bool doCompile = true;
    bool deleteSource = true;
    char* fileName = argc[1];
    char* outFileName;
    outFileName = (char*)malloc(1024);
    strncpy(outFileName,fileName,1000);
    strcat(outFileName,".o");
    bool isSetOut = false;
    //set flags 
    for(i=2;i<argv;i++){
        if(isSetOut){
            isSetOut = false;
            outFileName = argc[i];
        }
        else if(strcmp(argc[i],"-c")==0){
            doCompile = false;
        }
        else if(strcmp(argc[i],"-d")==0){
            deleteSource = false;
        }
        else if(strcmp(argc[i],"-o")==0){
            isSetOut = true;
        }
    }
    char outFileC[1024];
    strncpy(outFileC,outFileName,1000);
    strcat(outFileC,".c");

    //bf file
    FILE* bfFile;
    bfFile = fopen(fileName,"r");
    if(bfFile==NULL){
        printf("ERROR : FILE %s DOES NOT EXIST\n",fileName);
        return 2;
    }
    char c;

    //c source code
    FILE* cFile;
    cFile = fopen(outFileC,"w");
    int add = 0;
    char prevC = '\0';
    fputs("#include<stdio.h>\n#include<stdlib.h>\n",cFile);
    fputs("int main(){",cFile);
    fputs("unsigned char* _=(unsigned char*)malloc(32*1024);/*32kB*/if(_==0){printf(\"MEMORY ERROR!\\n\");return 1;}",cFile);
    //write codes   
    do{
        c = fgetc(bfFile);
        if(c!=EOF){
            bool isPrint = false;
            switch(c){
                case '>':
                case '<':
                    if((prevC=='+'||prevC=='-')&&add){//process something like +++- = ++
                        if(add==1||add==-1){
                            if(add==1) fputs("++*_",cFile);
                            else fputs("--*_",cFile);
                        }
                        else fprintf(cFile,"(*_)%c=%d",add>0?'+':'-',add>0?add:-add);
                        fputs(";",cFile);
                        add=0;
                    }
                    if(c=='>') add++;
                    else add--;
                    break;
                case '+':
                case '-':
                    if((prevC=='>'||prevC=='<')&&add){//process something like >>>< = >>
                        if(add==1||add==-1){
                            if(add==1) fputs("++_",cFile);
                            else fputs("--_",cFile);
                        }
                        else fprintf(cFile,"_%c=%d",add>0?'+':'-',add>0?add:-add);
                        fputs(";",cFile);
                        add=0;
                    }
                    if(c=='+') add++;
                    else add--;
                    break;
                case '.':
                case ',':
                case '[':
                case ']':
                    if(add){
                        if(prevC=='>'||prevC=='<'){//process something like >>>< = >>
                            if(add==1||add==-1){
                                if(c=='.'){
                                    isPrint = true;
                                    if(add==1) fputs("putchar(*(++_))",cFile);
                                    else fputs("putchar(*(--_))",cFile);
                                }else{
                                    if(add==1) fputs("++_",cFile);
                                    else fputs("--_",cFile);
                                }
                            }
                            else if(c=='.'){
                                isPrint = true;
                                fprintf(cFile,"putchar(*(_%c=%d))",add>0?'+':'-',add>0?add:-add);
                            }else fprintf(cFile,"_%c=%d",add>0?'+':'-',add>0?add:-add);
                            fputs(";",cFile);
                            add=0;
                        }else if(prevC=='+'||prevC=='-'){//process something like +++- = ++
                            if(add==1||add==-1){
                                if(c=='.'){
                                    isPrint = true;
                                    if(add==1) fputs("putchar(++*_)",cFile);
                                    else fputs("putchar(--*_)",cFile);
                                }else{
                                    if(add==1) fputs("++*_",cFile);
                                    else fputs("--*_",cFile);
                                }
                            }
                            else if(c=='.'){
                                isPrint = true;
                                fprintf(cFile,"putchar((*_)%c=%d)",add>0?'+':'-',add>0?add:-add);
                            }else fprintf(cFile,"(*_)%c=%d",add>0?'+':'-',add>0?add:-add);
                            fputs(";",cFile);
                            add=0;
                        }
                    }
                    switch(c){
                        case '.':
                            if(!isPrint) fputs("putchar(*_);",cFile);
                            break;
                        case ',':
                            fputs("*_=getchar();",cFile);
                            break;
                        case '[':
                            fputs("while(*_){",cFile);
                            break;
                        case ']':
                            fputs("}",cFile);
                            break;
                    }
                    break;
            }
        }
        if(c=='>'||c=='<'||c=='+'||c=='-'||c=='.'||c==','||c=='['||c==']') prevC = c;
    }while(c!=EOF);
    fputs("free(_);return 0;}",cFile);
    fclose(bfFile);
    fclose(cFile);
    if(!doCompile){
        printf("Output C code : %s\n",outFileC);
        return 0;
    }
    printf("Compile with GCC...\n");
    char op[2048] = "gcc ";
    strcat(op,outFileC);
    strcat(op," -o ");
    strcat(op,outFileName);
    system(op);
    if(deleteSource){
        strcpy(op,"rm ");
        strcat(op,outFileC);
        system(op);
    }
    printf("Done.\nOutput file name : %s\n",outFileName);
    return 0;
}
share|improve this question
add comment

3 Answers

up vote 16 down vote accepted
  1. Don't put everything in a giant main function, that just makes it hard to follow
  2. Print error message, such as usage information to stderr
  3. You have argc (argument count) and argv (argument vector) backwards
  4. You don't free anything you malloc
  5. There is no point in mallocing, because you can create your arrays on the stack
  6. Rather then passing a significantly lower value as your size to strncpy, use strncat for the concatenation.
  7. Repeated constants like the size of the strings you are using are best made #defines
  8. Since you appear to be using a recent version of C, don't split definitions and assignments onto seperate lines unneccesairly
  9. I prefer putting logic into else blocks rather then depending on return skipping the rest.
  10. Rather then calling rm, use the standard library function "unlink" for deleteing files
  11. Don't generate code using ++/-- it would won't help the final speed and makes your code more complicated
  12. Don't try to subtract instead of adding negative numbers, the compiler is smart enough to handle that.
  13. Why isn't the semicolon inside the printf?
  14. The name add isn't very clear. It took me a while to figure out what it was doing
  15. In general you spend a lot of effort trying to generate more compact C code. But the C compiler will be smarter at doing that then you are, so don't try.

Having cleaned up those issues I can see the problematic repetition of logic. The first thing is that all of non-bf characters are causing complications. We'll write function which calls fgetc but filters out all the characters we want to ignore.

Next, rather then trying to respond one character at a time, have loops which eat the characters.

Here is the result:

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

#define STRING_SIZE 1024

typedef struct CommandOptions
{
    bool doCompile;
    bool deleteSource;
    char * fileName;
    char outFileName[STRING_SIZE];
    char outFileC[STRING_SIZE];
}CommandOptions;

bool parse_command_line(CommandOptions * options, int argc, char * argv[])
{
    //help message
    if(argc==1){
        fprintf(stderr, "\n");
        fprintf(stderr, "How to use : %s filename [-o output|-c|-d|-r]\n",argv[0]);
        fprintf(stderr, "-o output : Set output file name\n-c : Do not compile\n-d : Do not delete C source file\n");
        fprintf(stderr, "INFO : You SHOULD type 'export MALLOC_CHECK_=0' manually to remove warning.\n");
        fprintf(stderr, "\n");
        return false;
    }

    // set the default options
    options->doCompile = true;
    options->deleteSource = true;
    options->fileName = argv[1];

    strncpy(options->outFileName, options->fileName, STRING_SIZE);
    strncat(options->outFileName, ".o", STRING_SIZE);

    // parse the remaining options
    int i;
    bool isSetOut = false;

    for(i = 0; i < argc; i++) 
    {
        if(isSetOut){
            isSetOut = false;
            strncpy(options->outFileName, argv[i], STRING_SIZE);
        }
        else if(strcmp(argv[i],"-c")==0){
            options->doCompile = false;
        }
        else if(strcmp(argv[i],"-d")==0){
            options->deleteSource = false;
        }
        else if(strcmp(argv[i],"-o")==0){
            isSetOut = true;
        }
    }

    strncpy(options->outFileC,options->outFileName,STRING_SIZE);
    strncat(options->outFileC,".c", STRING_SIZE);

    // don't delete the source if we won't compile it
    if(!options->doCompile)
    {
        options->deleteSource = false;
    }

    return true;

}

void write_header(FILE * cFile)
{
    fputs("#include<stdio.h>\n", cFile);
    fputs("#include<stdlib.h>\n",cFile);
    fputs("int main(){\n",cFile);
    fputs("unsigned char* _=(unsigned char*)malloc(32*1024);/*32kB*/if(_==0){printf(\"MEMORY ERROR!\\n\");return 1;}\n",cFile);
}

void write_footer(FILE * cFile)
{
    fputs("free(_);\nreturn 0;\n}\n",cFile);
}

int bf_fgetc(FILE * bfFile)
{
    int c;
    do
    {
        c = fgetc(bfFile);
    }while(c != EOF && c != '[' && c != ']' && c != '<' && c != '>' && c != '.' 
        && c != ',' && c != '+' && c != '-');
    return c;
}

void compile_to_c(FILE * bfFile, FILE * cFile)
{
    write_header(cFile);
    int add = 0;
    char prevC = '\0';
    //write codes   
    char c = bf_fgetc(bfFile);
    do{
        int movement_counter = 0;
        while( c == '>' || c == '<')
        {
            movement_counter += c == '>' ? 1 : -1;
            c = bf_fgetc(bfFile);
        }
        if(movement_counter)
        {
            fprintf(cFile,"_ += %d;", movement_counter);
        }

        int value_counter = 0;
        while( c == '+' || c == '-')
        {
            value_counter += c == '+' ? 1 : -1;
            c = bf_fgetc(bfFile);
        }
        if(value_counter)
        {
            fprintf(cFile,"*_ += %d;",value_counter);
        }

        if(c == '.') 
        {
            fprintf(cFile, "putchar(*_);\n"); 
            c = bf_fgetc(bfFile); 
        }
        if(c == ',') 
        {
            fprintf(cFile, "*_ = getchar();\n");
            c = bf_fgetc(bfFile); 
        }
        if(c == '[')
        {
            fprintf(cFile, "while(*_) {\n");
            c = bf_fgetc(bfFile);
        }
        if(c == ']')
        {
            fprintf(cFile, "}\n");
            c = bf_fgetc(bfFile);
        }
    }while(c!=EOF);
    write_footer(cFile);
}

void compileCode(CommandOptions * options)
{
    printf("Compile with GCC...\n");
    char op[2048] = "gcc ";
    strncat(op,options->outFileC, 2048);
    strncat(op," -o ", 2048);
    strncat(op,options->outFileName, 2048);
    system(op);
}

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

    if( !parse_command_line(&options, argc, argv) )
    {
        return 1;
    }

    printf("INFO : You may type 'export MALLOC_CHECK_=0' manually to remove warning.\n");

    //bf file
    FILE* bfFile = fopen(options.fileName,"r");
    if(bfFile==NULL){
        fprintf(stderr, "ERROR : FILE %s DOES NOT EXIST\n",options.fileName);
        return 2;
    }

    //c source code
    FILE* cFile = fopen(options.outFileC, "w");
    if(!cFile)
    {
        fclose(bfFile);
        fprintf(stderr, "ERROR: COULD NOT OPEN %s FILE FOR WRITING" , options.outFileC);
        return 3;
    }

    compile_to_c(bfFile, cFile);

    fclose(bfFile);
    fclose(cFile);

    if(options.doCompile){
        compileCode(&options);
    }else{
        printf("Output C code : %s\n", options.outFileC);
    }

    if( options.deleteSource )
    {
        unlink(options.outFileC);
    }

    printf("Done.\nOutput file name : %s\n",options.outFileName);
    return 0;
}

There is still some duplication in the compile_to_c function that I'm not liking. The similarity between <> and +- could easily be exploited easily in a function. Harder would be getting rid of the repeated bf_fgetc calls.

share|improve this answer
    
Thanks! ...I should learn how to use structure and functions properly... –  JiminP May 18 '11 at 3:14
    
@JiminP, I recently wrote a BF interpreter and I remembered an optimization I had that you might be useful. During preprocessing I identified "trivial loops." A trivial loop consists only of <>+-, has balanced <> (so it ends up where it started), and results in decrementing the first value (the one that gets tested by [) exactly once. This means I can tell how many iterations to loop will go through right away and can replace the whole loop with one iteration using multiplication to apply the same effect as if it the loop had run many times. –  Winston Ewert May 18 '11 at 15:01
add comment

You have two memory management bugs.

Memory returned from malloc() is, in general, not initialized to zero. A good operating system will zero all memory pages that are given to your program (to prevent information leakage from other programs), and your program's first malloc() would likely obtain virgin memory. However, none of that is guaranteed by POSIX. I suggest calling calloc() instead.

The pointer that you free() does not necessarily have the same value that you received from malloc(). The pointer can take on any value as it moves among the cells. If the pointer does not happen to be at cell 0 when the program runs out of instructions to execute, then you end up freeing an arbitrary pointer, which is a pity when you're so close to finishing successfully!

Possible remedies are:

  • Save a copy of the value returned from malloc() so that you can free() it.
  • Don't bother calling free(); just let the OS take care of the cleanup.
  • Since you are only doing a fixed-size allocation, just use an array: unsigned char memory[32 * 1024] ={ 0 }, *_ = memory;
share|improve this answer
add comment

Don't cast malloc. It's unnecessary in C, and can hide a bug if you don't have a valid prototype in scope (ie. you didn't include stdlib.h)

share|improve this answer
    
I know, why would anyone ever need dynamic memory allocation, right.....? –  Ed S. Aug 22 '11 at 6:35
1  
@Ed. S he said cast not call –  Winston Ewert Sep 5 '11 at 14:38
    
@Winston... so he did, sorry about that. –  Ed S. Sep 6 '11 at 16:22
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.