Brainfuck interpreter in C 3

Question

I created my brainfuck interpreter and I would like to know what can be done better and is the code clear and readable. I will be very thankful for opinions and suggestions. Here is the code:

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

#define ALLOCATION_ERROR 1
#define FILE_ERROR 2
#define OTHER_ERROR 3
#define TAPE_SIZE 30000

FILE* get_file_handle(const char* filename){
    FILE* input_file=fopen(filename,"rb");
    if(input_file==NULL){
        fprintf(stderr,"Error: failed to open file %s\n",filename);
        exit(FILE_ERROR);
    }
    return input_file;
}
unsigned char* read_code(FILE* input_file){
    fseek(input_file,0,SEEK_END);
    size_t code_size=(size_t)ftell(input_file);
    fseek(input_file,0,SEEK_SET);
    unsigned char* code=malloc(code_size+1);
    if(code==NULL){
        fprintf(stderr,"Fatal: failed to allocate %zu bytes.\n",code_size+1);
        exit(ALLOCATION_ERROR);
    }
    if(fread(code,1,code_size,input_file)!=code_size){
        perror("Error: failed to read from file\n");
        exit(FILE_ERROR);
    }
    code[code_size]=0;
    return code;
}
unsigned char* create_tape(){
    unsigned char* tape=calloc(TAPE_SIZE,1);
    if(tape==NULL){
        fprintf(stderr,"Fatal: failed to allocate %zu bytes.\n",(size_t)TAPE_SIZE*1);
        exit(ALLOCATION_ERROR);
    }
    return tape;
}
void find_matching_bracket(unsigned char** tape_ptr,unsigned char** code_ptr){
    int is_right_bracket=']'==**code_ptr;
    if(is_right_bracket?**tape_ptr:!**tape_ptr){
        int loop=1;
        while(loop){
            is_right_bracket?--*code_ptr:++*code_ptr;
            if(**code_ptr=='[')
                is_right_bracket?--loop:++loop;
            if(**code_ptr==']')
                is_right_bracket?++loop:--loop;
        }
    }
}
void run(const char* filename){
    FILE* input_file=get_file_handle(filename);
    unsigned char *tape=create_tape(),*tape_ptr=tape;
    unsigned char *code=read_code(input_file),*code_ptr=code;
    fclose(input_file);
    for(;*code_ptr;++code_ptr){
        switch(*code_ptr){
            case '>':
                ++tape_ptr;
                break;
            case '<':
                --tape_ptr;
                break;
            case '+':
                ++*tape_ptr;
                break;
            case '-':
                --*tape_ptr;
                break;
            case ',':
                *tape_ptr=(unsigned char)getchar();
                break;
            case '.':
                putchar(*tape_ptr);
                fflush(stdout);
                break;
            case '[':
            case ']':
                find_matching_bracket(&tape_ptr,&code_ptr);
                break;
        }
    }
    free(tape);
    free(code);
}
int main(int argc,char** argv){
    if(argc!=2){
        puts("Usage: bfic <source>");
        exit(OTHER_ERROR);
    }
    run(argv[1]);
}

Code after review from @luserdroog and @AustinHastings:

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

#define ALLOCATION_ERROR 1
#define FILE_ERROR 2
#define OTHER_ERROR 3
#define TAPE_SIZE ((size_t)300000)

static inline FILE*
get_file_handle(const char* filename){
    FILE* input_file=fopen(filename,"rb");
    if(input_file==NULL){
        fprintf(stderr,"Error: failed to open file %s\n",filename);
        exit(FILE_ERROR);
    }
    return input_file;
}
static inline unsigned char*
read_code(FILE* input_file){
    fseek(input_file,0,SEEK_END);
    size_t code_size=(size_t)ftell(input_file);
    fseek(input_file,0,SEEK_SET);
    unsigned char* code=malloc(code_size+1);
    if(code==NULL){
        fprintf(stderr,"Fatal: failed to allocate %zu bytes.\n",code_size+1);
        exit(ALLOCATION_ERROR);
    }
    if(fread(code,1,code_size,input_file)!=code_size){
        perror("Error: failed to read from file\n");
        exit(FILE_ERROR);
    }
    code[code_size]=0;
    return code;
}
static inline unsigned char*
create_tape(){
    unsigned char* tape=calloc(TAPE_SIZE,1);
    if(tape==NULL){
        fprintf(stderr,"Fatal: failed to allocate %zu bytes.\n",TAPE_SIZE*1);
        exit(ALLOCATION_ERROR);
    }
    return tape;
}
static inline void
find_matching_bracket(unsigned char** tape_ptr,unsigned char** code_ptr){
    int is_right_bracket=']'==**code_ptr;
    if(is_right_bracket?**tape_ptr:!**tape_ptr){
        int depth=1;
        while(depth>0){
            is_right_bracket?--*code_ptr:++*code_ptr;
            if(**code_ptr=='[')
                is_right_bracket?--depth:++depth;
            if(**code_ptr==']')
                is_right_bracket?++depth:--depth;
        }
    }
}
static inline void
run(const char* filename){
    FILE* input_file=get_file_handle(filename);
    unsigned char* tape=create_tape();
    unsigned char* tape_ptr=tape;
    unsigned char* code=read_code(input_file);
    unsigned char* code_ptr=code;
    fclose(input_file);
    for(;*code_ptr;++code_ptr){
        switch(*code_ptr){
            case '>':
                ++tape_ptr;
                break;
            case '<':
                --tape_ptr;
                break;
            case '+':
                ++*tape_ptr;
                break;
            case '-':
                --*tape_ptr;
                break;
            case ',':
                *tape_ptr=(unsigned char)getchar();
                break;
            case '.':
                putchar(*tape_ptr);
                fflush(stdout);
                break;
            case '[':
            case ']':
                find_matching_bracket(&tape_ptr,&code_ptr);
                break;
        }
    }
    free(tape);
    free(code);
}
static inline void
parse_args(int argc){
    if(argc!=2){
        puts("Usage: bfic <source>");
        exit(OTHER_ERROR);
    }
}
int
main(int argc,char** argv){
    parse_args(argc);
    run(argv[1]);
}

Answer 1

CodeReview problems

You've made two "codereview" errors (as opposed to "coding" errors):

Use tags

You didn't specify enough about your environment. What version of C are you writing for? (I'm guessing "not K&R" since you're using ANSI-style function declarations. But is that C89, C99, C11, or C18?) Is your code limited to *nix or Windows, or must it run on both? Do you care about compiler versions? Are you allowed to use compiler extensions?

No moving targets!

You edited your code after posting it. Someone is going to yell at you for that - it's considered poor form. Since nobody had replied when you edited, it's not the end of the world, though.

Coding style

I have some issues with your coding style. You didn't specify what style you were trying to write, and I suspect you started with "IOCCC" as your base, although I don't know why. So:

Find a style

There are three kinds of "C Coding Style" guides out there:

The first kind are actually C++ guides. Ignore these.

The second kind are "we want to have a guide, but we don't want to risk a flamewar, so we're afraid to make any firm guidelines". Ignore these, too.

The third kind will provide some good advice, and some amount of nutrient-laden fertilizer. That's the kind you want! Find one of those you like and stick with it.

I don't care if your tabs are 2 spaces, 11 spaces or what, as long as they stay the same. I don't care if your braces are up or down, or even (God forbid) down and indented. The good advice and consistency will overwhelm your failure to use a multiple of a perfect prime number as your tab size.

Once you find your ideal coding standard, tattoo it on your body somewhere. I have found that whatever style people adopt first they will be able to rationalize retaining for the rest of their lives. And you can be really dogmatic about it - it's fine! I still write (C) code using the coding standard from my first "corporate" job by default. (Of course, it was a pretty well-thought-out standard, even if pre-ANSI...)

Having a formal document to refer to makes it really easy to be dogmatic. Go ahead, it's the internet!

Whitespace is free. Use lots of it.

I'm not aware of any coding style guide that argues for the elimination of whitespace. If you are following one, please post a link to it so that we can burn it out for the heresy that it is! This code shows a lack of horizontal and vertical spacing:

        code[code_size]=0;
        return code;
    }
    unsigned char* create_tape(){

It should be:

        code[code_size] = 0;
        return code;
    }

    unsigned char *
    create_tape()
    {

(Although you might want to "cuddle" that opening brace... if you're a heretic.)

Pick better names

This is a bit of an art form, but what does get_file_handle return? Surprisingly, it returns a FILE pointer. This is surprising because handle is one of those magic words in computing that "everybody knows" what it means. And it doesn't mean that. According to Jargon a handle is:

2. A magic cookie, often in the form of a numeric index into some array somewhere, through which you can manipulate an object like a file or window. The form file handle is especially common.

Also, what does "get" mean? (Seriously.) There are quite a few meanings for get in computing. Java(Beans) screwed it up for most people by slathering it in front of their accessors. But it used to mean "fetch or create." Now days, you're better off avoiding it, especially considering what your function does: return a valid file handle or die.

I'd suggest either explicitly stating that in your function name (open_or_die), or simply echoing the "successful" behavior by calling it something like fopen_rb or open_file_rb.

FILE *
open_or_die(filename)
    const char *filename;
{
    FILE *input = fopen(filename, "rb");

    if (input)
        return input;

    fprintf(stderr, "Error: failed to open file %s\n", filename);
    exit(FILE_ERROR);
}

Finally, what does loop mean? Maybe nested or levels or depth would be better. Or even num_open or open_brackets.

Create the right functions

There are three reasons to create a function from some non-function code.

1. Create a function for things you do more than one time. In my opinion, N=2 is the right time to create a function. Sometimes you'll find yourself doing N>2, but IMO 2 is the right number. (For example, some trivial pair of calls to configure buttons in a GUI might seem "simple enough" to not make a function. Make the function!)

   In your code, you have three different places where you exit if a pointer is NULL. That would fall under the rule of N > 1, and so you might write something like:

   void die(const char * fmt, ...);
   
   // ...
   
   input = fopen(filename, "rb") 
       || die("Could not open input file '%s' for reading", filename);
   

   (Note: I don't suggest doing that, because of #2 here.)

2. Create a function to "abstract away" code to a separate layer. In your main, you have:

   int main(int argc,char** argv){
       if(argc!=2){
           puts("Usage: bfic <source>");
           exit(OTHER_ERROR);
       }
       run(argv[1]);
   }
   

   The first paragraph (the if statement) is at a much lower level of detail than the run call. I would be inclined to write something like:

   parse_args(argc, argv);
   run_code(argv[1]);
   

   despite the "trivial" nature of the parse_args, because that puts them both at the same level of abstraction. Alternatively, you might pull out some of the code from run to put more meat into main like:

   const char *code_file = parse_args(argc, argv);
   const code_t *code = load_code(code_file);
   int result = run_code(code);
   return result;
   

You do this with your read_code and create_tape functions, and this is generally the right way to go if you have to choose between #1 (above) and #2 (this option).

3. Create a function to isolate a potentially valuable operation for reuse.

   This is the gold standard for functions, and so you won't see it very often. But when you do see it, grab on! In your case, you've got a couple of them right here:

   fseek(input_file,0,SEEK_END);
   size_t code_size=(size_t)ftell(input_file);
   fseek(input_file,0,SEEK_SET);
   

   This code computes the size of a file given a file pointer. That's something you might wish to reuse later, and it has nothing to do with your main code - there are no special types or anything getting in the way. This would be something you could put in your toolbox. (You'll probably find half a dozen different ways to do this task. Collect 'em all!)

   Also, consider that your read_code function represents an operation so common that Perl gives it a name:

   slurp

   To read an entire file into a string in one operation.

   That's another function that would be worth "isolating" so you could keep it in your toolbox. (By "isolating" I mean separating it from your code, so that the name is more general, the types used are all standard types, etc.)

Code organization

The one weak spot I think I see with your code organization is writing your code with a common level of abstraction inside functions. I've already mentioned main. This also shows up in run in two ways. First, because the interpreter code is finer grained than the make_tape and read_code functions, and second because read_code should just go ahead and incorporate the file open and close operations. Change this:

FILE* input_file=get_file_handle(filename);
unsigned char *tape=create_tape(),*tape_ptr=tape;
unsigned char *code=read_code(input_file),*code_ptr=code;
fclose(input_file);

... to this:

unsigned char *tape = create_tape();
unsigned char *tape_ptr = tape;
unsigned char *code = read_code(filename);
unsigned char *code_ptr = code;

... by moving the open/close operations down into read_code. And adding newlines.

Final notes

You spend some characters casting the value of TAPE_SIZE. Why not either include the typecast in the macro (#define TAPE_SIZE ((size_t)300000)) or declare a static variable for the value which you could configure from the command line (-t SIZE):

size_t Tape_size = TAPE_SIZE;

You also mentioned in the comments on your request a performance hit after converting to functions. If you look at your original version, you handled '[' and ']' separately. Why not do that in your functions? And while you're at it, just return the result.

const unsigned char *
find_matching_bracket(tape_ptr, code_ptr)
    const unsigned char *tape_ptr;
    const unsigned char *code_ptr;
{
    if (*code_ptr == ']') {
        // search this-a-way
    }
    else {
        // search that-a-way
    }

    return code_ptr;
}

Answer 2

First let me say "Good work!" on revising the code to use functions. This is important in C (and most other languages, too) because it makes smaller units of code which are easier to read and reason about.

As you have discovered, making more function calls has the potential to make the program slower if the compiler is not doing significant optimizations. Modern optimizing compilers can automatically inline functions when the optimizations are turned on, but you can also suggest to the compiler that it can inline the functions.

If you change the function prototype from, eg.

void find_matching_bracket(unsigned char** tape_ptr,unsigned char** code_ptr){

to

static inline
void find_matching_bracket(unsigned char** tape_ptr,unsigned char** code_ptr){

that should help with the speed. Especially this function which is in the "inner loop" should have a significant benefit from being inlined.

static isn't strictly necessary here, but it often goes well with inline.

Another option is to turn up the compiler optimization level. With gcc or clang, you can add -O2 or -O3 and it should do the inlining for you.

Btw, your find_matching_bracket function looks better than the one I wrote for my interpreter. It's shorter and simpler.

(out of time. more to add later)