Basic Brainfuck Interpreter in C I have written a small Brainfuck interpreter in C. More recent version hosted here.

I have found it very hard to test my interpreter because writing a program in Brainfuck is somewhat hard (or at least very tedious) and many snippets I found online either don't give output or do so very cryptically, so I don't know 100% sure if everything works as intended.

I have tested a few programs on it though, and that which I tested works.

Any criticism/advice would be very nice.

#include &lt;stdio.h&gt;

#define MEMSIZE 30000

int memory[MEMSIZE];
int instructions[MEMSIZE];

int memptr = 0;
int insptr = 0;

int isInstruction(char);
void runInstruction(char);

int main(int argc, char *argv[]) {
    // Open input file
    FILE *file = fopen(argv[1], "r");
    if (file == 0){
        printf("Error: Could not open source file\n");
        return 0;
    }

    // Read all instructions from input
    char c;
    int i = 0;
    int balance = 0; // Keeps track of `[` and `]` balance
    while ((c = fgetc(file)) != EOF &amp;&amp; i &lt; MEMSIZE){
        if(isInstruction(c)){
            instructions[i] = c;
            i++;
            if (c == '[')
                balance++;
            if (c == ']')
                balance--;
        }
    }

    // Close input file
    fclose(file);

    // Error if amount of instructions is more than permitted
    if (c != EOF &amp;&amp; i &gt;= MEMSIZE){
        printf("Error: cannot read more than 30000 instructions\n");
        return 0;
    }

    // Error if `[` and `]` aren't balanced
    if (balance != 0){
        printf("Error: `[` and `]` were not balanced\n");
        return 0;
    }

    // Run the program
    while(insptr &lt; MEMSIZE &amp;&amp; instructions[insptr] != 0){
        runInstruction(instructions[insptr]);
        insptr++;
    }
}

int isInstruction(char c) {
    if(c == '+' || c == '-' || c == '&gt;' || c == '&lt;' || c == '[' || c == ']' ||
       c == '.' || c == ','){
        return 1;
    } else {
        return 0;
    }
}

void runInstruction(char c) {
    switch(c){
    case '+':
        memory[memptr]++;
        break;
    case '-':
        memory[memptr]--;
        break;
    case '&gt;':
        if (memptr &lt; MEMSIZE - 1)
            memptr++;
        else
            memptr = 0;
        break;
    case '&lt;':
        if (memptr &gt; 0)
            memptr--;
        else
            memptr = MEMSIZE - 1;
        break;
    case '[':
        if (memory[memptr] == 0)
            // seek `]` forward
            while(instructions[insptr] != ']'){
                insptr++;
            }
        break;
    case ']':
        if (memory[memptr] != 0)
            // seek `[` backward
            while(instructions[insptr] != '['){
                insptr--;
            }
        break;
    case '.':
        putchar(memory[memptr]);
        break;
    case ',':
        memory[memptr] = getchar();
        break;
    default:
        printf("Error: Encountered unknown instruction `%c`\n",c);
    }
}
Use NULL for null pointers

NULL conveys your intention to use a pointer better than 0:

if (file == NULL) { ... }


As a bonus, in C the macro NULL typically expands to something like (void*)0; if you have warnings for potentially unsafe type conversions enabled (-Wconversion for GCC), the compiler will notify you, if you try to assign 0 to a pointer or NULL to an integer, which helps to find potential errors.

Use puts when you don't use string formatting

printf("Error: Could not open source file\n");


could just as well be

puts("Error: Could not open source file");


While printf is great for formatted output, the look for formatting specifiers carries a computational overhead that puts doesn't have. Some compilers (like GCC) will perform that optimization automatically for you, if you use a format string literal without format specifiers.

Checking whether a characters is in a set of characters

In isInstruction(char), instead of using a series of disjoint (joint with the || operator) equality comparisons, you could much better express your intent and the source character set with strchr:

return strchr("+-&gt;&lt;[].,", c) != NULL;


A possible improvements might be:

const static char brainfuck_alphabet[] = {'+', '-', '&gt;', '&lt;', '[', ']', '.', ','};
return memchr(brainfuck_alphabet, c, sizeof(brainfuck_alphabet));


The improvements lies in the fact that the algorithm of memchr can make an assumption about the size of the memory region to check while strchr cannot. This could lead to a performance improvement.

Use bool for boolean values

The header &lt;stdbool.h&gt; declares boolean-like types. You can use them to express your intention for a boolean return value of isInstruction():

bool isInstruction(char c) { ... }


Don't repeat yourself

While you generally did will to adhere to that principle, I want to point out one small spot that could use improvement:

printf("Error: cannot read more than 30000 instructions\n");


could be

printf("Error: cannot read more than %d instructions\n", MEMSIZE);


That way the error message would reflect the actual buffer size in case MEMSIZE was redefined. Since the argument list of printf is variadic and doesn't carry type information, it would be better yet to not make assumptions on the width of the integer type of MEMSIZE. What if someone defined MEMSIZE to be larger than MAX_INT? The best course of action would be to force MEMSIZE into the type size_t (more about that later).

printf("Error: cannot read more than %zu instructions\n", (size_t)(MEMSIZE));


Data types


You only ever store chars in instructions, so its type could just as well be:

char instructions[…];


On all 32- and 64-bit platforms you just saved 3*MEMSIZE bytes (which you could use to increase MEMSIZE again if that is a concern).
memptr, insptr and i are indexes of plain arrays. Therefore they should have type size_t, which is the type meant to reflect the size or position of things in memory.


Make the code flow of branches based on mutually exclusive conditions mutually exclusive

In the above code the conditions c == '[' and c == ']' cannot both be true since c isn't altered between them. To make this obvious to the reader and the compiler (though a good one should find that optimization itself), you should use mutually exclusive if … else if … statements:

        if (c == '[')
            balance++;
        else if (c == ']')
            balance--;


Exit with an error status in case of an error

Programs may convey information in their exit status. By convention the status 0 means “no error, everything performed according to your instructions”. Every other status value usually refers to an error condition but there is no general convention on what each value means. One attempt to conventionalize them can be found in the sysexits.h header. You could use it like this in your program:

if (file == NULL) {
    // ...
    return EX_NOINPUT;
}

if (balance &lt; 0) {
    // ...
    return EX_DATAERR;
}

if (c != EOF &amp;&amp; i &gt;= MEMSIZE) {
    // ...
    return EX_SOFTWARE;
}


In a similar fashion you could return an error from runInstruction(), but we already know that we didn't accept any illegal instructions.

Print error messages on stderr

printf and puts write their data to stdout, the standard output stream, but you're also writing program output to stdout via putchar. To avoid mixing program output and error messages, you should write the latter to stderr instead:

fprintf(stderr, ...);
fputs(..., stderr);


Be aware that fputs doesn't append a new-line character like puts.

Print operating system error messages to provide hints to the user

Like many library functions relying on system calls, fopen sets the global errno variable to report the type of error. You can use functions like strerror or perror to decode the value of errno into a human-readable, localized(!) error message:

if (file == NULL) {
    perror("Could not open source file");
    // ...
}


Respect the principle of locality, avoid global variables

Global variables clutter the name space and make it more difficult to follow the flow of the program. They also prevent a method or set of methods from being reentrant, a property that allows them to be executed concurrently. (This matters not just for multi-threaded programs, but also if you wanted to, say, interpret multiple Brainfuck programs one instructions at a time in a round-robin fashion to see which program terminates first.)

One way to achieve this is to move more data and logic into the method:

int main(int argc, char *argv[])
{
    // ...
    char instructions[MEMSIZE];
    // ...
    runInstructions(instructions, i);

    return EX_OK;
}

void runInstructions(const char *const instructions, size_t instruction_count)
{
    size_t memptr = 0;
    size_t insptr = 0;
    int memory[MEMSIZE];

    while (insptr &lt; instruction_count) {
        switch(instructions[insptr]) {
            // ...
        }
    }
}


You could also use an instruction pointer instead of an instruction index:

void runInstructions(const char *const instructions, size_t instruction_count)
{
    size_t memptr = 0;
    const char *insptr = instructions;
    const char *const instructions_end = instructions + instruction_count;
    int memory[MEMSIZE];

    while (insptr &lt; instructions_end) {
        switch(*insptr) {
        // ...
        case '[':
            if (memory[memptr] == 0)
                // seek `]` forward
                while (*insptr != ']')
                    insptr++;
            break;

        case ']':
            if (memory[memptr] != 0)
                    // seek `[` backward
                    while(*insptr != '[')
                        insptr--;
            break;
        // ...
        }
    }
}


Use library functions where possible

Library functions unburden you from writing your own code. They're (usually) less likely to contain errors than hand-rolled solutions and the library authors may have included arcane, platform-specific performance optimizations in them.


To find the next closing bracket instruction you could write:

    case '[':
        if (memory[memptr] == 0) {
            // seek `]` forward
            insptr = memchr(insptr + 1, ']', (size_t)(instructions_end - insptr - 1));
        }
        break;

To find the last opening bracket you could write:

    case ']':
        if (memory[memptr] != 0) {
            // seek `[` backward
            insptr = memrchr(instructions, '[', (size_t)(insptr - instructions - 1));
        }
        break;



Avoid undefined behaviour


You can't be sure that argv[1] is defined. If it isn't its value may have NULL or garbage content. Check the value of argc first and print an appropriate error message if it's too small:

if (argc &lt; 2) {
    fputs("Error: No source file specified.\n", stderr);
    return EX_USAGE;
}

If the Brainfuck program happens to have a branch depth greater than INT_MAX, the balance variable would overflow. It typically overflows to the value -1, which would be caught with a subsequent balance &lt; 0 check, but in C signed integer overflow results in undefined behaviour. That means the compiler may assume that overflow never happens and optimize the negative balance check to always be false, which would leave your program in a weird state.

If you decide to care about that (INT_MAX+1 is 2^31 which would require a program of at least 2 GiB, all [, to trigger the bug on modern architectures after all), there are basically two approaches:


Use ssize_t instead of int. ssize_t covers a branching depth of almost (half) as many levels as the address space has bytes. It's quasi-impossible to generate and load a program of that size into the memory of your interpreter anyway, so you could consider your bases covered.
Check for overflow before increasing balance:

    if (c == '[') {
        if (balance == INT_MAX) {
            fprintf(stderr, "Error: Cannot handle branches deeper than %d levels.\n", INT_MAX);
            return EX_DATAERR;
        }
        balance++;
    }




Provide some debugging info to the programmer using your interpreter

It might be helpful for the user to know where in the program a parser error occurred, so you should keep track of the amount of bytes read so far:

size_t i = 0;
while (i &lt; MEMSIZE &amp;&amp; (c = fgetc(file)) != EOF) {
    read++;
    if(isInstruction(c)) { ... }
}


You can now report that value in parser error messages like so:

fprintf(stderr, "Error:%zu: Cannot handle branches deeper than %d levels.\n", read, INT_MAX);
fprintf(stderr, "Error:%zu: Encountered `]` before matching `[`\n", read);
fprintf(stderr, "Error:%zu: cannot read more than %zu instructions\n", (size_t)(MEMSIZE), read + 1);