Brainfuck to C converter

Question

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

Answer 1

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.

Answer 2

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;

Answer 3

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)