Could you please give this piece of code a review?

[Gary Willoughby]

This is a simple Brainfuck interpreter that i wrote using PellesC to learn C. I've been programming for a while but this is my first time learning C.

Can someone please take a look at the following code and give me a critical review? I just want to know that i'm heading in the right direction. When i mean review, i mean about the code and if it is 'good' C not if my interpreter could be done differently, as i'm sure it could. The program works and interprets Brainfuck code very well. I just want to know that i'm not abusing the C language.   Thanks.

Interpreter:

Code: [Select]

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

//Define the maximum size of the source buffer.
#define SOURCE_BUFFER_SIZE 100000

//This is the struct which models a loop pair.
typedef struct Loop
{
int Start;
int End;
} Loop;

//Print the usage information.
void PrintUsage(char *Name)
{
printf("USAGE: %s SourceFile\n", Name);
}

//The main stack in which programs function.
//30k as per the brainfuck standard.
char stack[30000]; char *p_stack = stack;

//Stack to track loops.
Loop Loops[1000];
int LoopIndex = 0;
int LoopStarts[1000];
int LoopStartIndex = 0;

//Source buffer, this contains the entire source code.
//1Mb should be enough.
char source[SOURCE_BUFFER_SIZE]; char *p_source = source;

//The current symbol in the source.
char symbol;

//Main.
int main(int argc, char *argv[])
{
//No source has been passed to the interpreter so give a error and quit.
if(argc <= 1)
{
fputs("ERROR: No source file specified.\n", stderr);

PrintUsage(argv[0]);
exit(0);
}

//Check if the passed argument was a question mark.
//If so show the usage information.
if(strlen(argv[1]) <= 2 && strchr(argv[1], '?') != NULL)
{
PrintUsage(argv[0]);
exit(0);
}

//The argument seems to be a text file, so lets open and read it.
FILE *fp = fopen(argv[1], "r");
if(fp != NULL)
{
//Lets read the source and load it into the source buffer.
fread(source, sizeof(char), SOURCE_BUFFER_SIZE - 1, fp);
fclose(fp);

//Record the positions of the loops.
for(int x = 0; (symbol = source[x]) != '\0'; x++)
{
switch (symbol)
{
case '[':
LoopStarts[LoopStartIndex++] = x;
break;
case ']':
Loops[LoopIndex].Start = LoopStarts[--LoopStartIndex];
Loops[LoopIndex].End = x;
LoopIndex++;
break;
}
}

//Interpret the source.
for(int x = 0; (symbol = source[x]) != '\0'; x++)
{
switch (symbol)
{
case '>':
++p_stack;
break;
case '<':
--p_stack;
break;
case '+':
++*p_stack;
break;
case '-':
--*p_stack;
break;
case '.':
putchar(*p_stack);
break;
case ',':
*p_stack = getchar();
break;
case '[':
if(*p_stack == '\0')
{
for(int y = 0; Loops[y].Start != '\0'; y++)
{
if(Loops[y].Start == x)
{
x = Loops[y].End;
}
}
}
break;
case ']':
if(*p_stack != '\0')
{
for(int y = 0; Loops[y].End != '\0'; y++)
{
if(Loops[y].End == x)
{
x = Loops[y].Start;
}
}
}
break;
}
}
}
else
{
fputs("ERROR: Can not open file.\n", stderr);
}

    return 0;
}

Sample Brainfuck program:

Code: [Select]

>+++++++++[<+++++++++++>-]<[>[-]>[-]<<[>+>+<<-]>>[<<+>>-]>>>
[-]<<<+++++++++<[>>>+<<[>+>[-]<<-]>[<+>-]>[<<++++++++++>>>+<
-]<<-<-]+++++++++>[<->-]>>+>[<[-]<<+>>>-]>[-]+<<[>+>-<<-]<<<
[>>+>+<<<-]>>>[<<<+>>>-]>[<+>-]<<-[>[-]<[-]]>>+<[>[-]<-]<+++
+++++[<++++++<++++++>>-]>>>[>+>+<<-]>>[<<+>>-]<[<<<<<.>>>>>-
]<<<<<<.>>[-]>[-]++++[<++++++++>-]<.>++++[<++++++++>-]<++.>+
++++[<+++++++++>-]<.><+++++..--------.-------.>>[>>+>+<<<-]>
>>[<<<+>>>-]<[<<<<++++++++++++++.>>>>-]<<<<[-]>++++[<+++++++
+>-]<.>+++++++++[<+++++++++>-]<--.---------.>+++++++[<------
---->-]<.>++++++[<+++++++++++>-]<.+++..+++++++++++++.>++++++
++[<---------->-]<--.>+++++++++[<+++++++++>-]<--.-.>++++++++
[<---------->-]<++.>++++++++[<++++++++++>-]<++++.-----------
-.---.>+++++++[<---------->-]<+.>++++++++[<+++++++++++>-]<-.
>++[<----------->-]<.+++++++++++..>+++++++++[<---------->-]<
-----.---.>>>[>+>+<<-]>>[<<+>>-]<[<<<<<.>>>>>-]<<<<<<.>>>+++
+[<++++++>-]<--.>++++[<++++++++>-]<++.>+++++[<+++++++++>-]<.
><+++++..--------.-------.>>[>>+>+<<<-]>>>[<<<+>>>-]<[<<<<++
++++++++++++.>>>>-]<<<<[-]>++++[<++++++++>-]<.>+++++++++[<++
+++++++>-]<--.---------.>+++++++[<---------->-]<.>++++++[<++
+++++++++>-]<.+++..+++++++++++++.>++++++++++[<---------->-]<
-.---.>+++++++[<++++++++++>-]<++++.+++++++++++++.++++++++++.
------.>+++++++[<---------->-]<+.>++++++++[<++++++++++>-]<-.
-.---------.>+++++++[<---------->-]<+.>+++++++[<++++++++++>-
]<--.+++++++++++.++++++++.---------.>++++++++[<---------->-]
<++.>+++++[<+++++++++++++>-]<.+++++++++++++.----------.>++++
+++[<---------->-]<++.>++++++++[<++++++++++>-]<.>+++[<----->
-]<.>+++[<++++++>-]<..>+++++++++[<--------->-]<--.>+++++++[<
++++++++++>-]<+++.+++++++++++.>++++++++[<----------->-]<++++
.>+++++[<+++++++++++++>-]<.>+++[<++++++>-]<-.---.++++++.----
---.----------.>++++++++[<----------->-]<+.---.[-]<<<->[-]>[
-]<<[>+>+<<-]>>[<<+>>-]>>>[-]<<<+++++++++<[>>>+<<[>+>[-]<<-]
>[<+>-]>[<<++++++++++>>>+<-]<<-<-]+++++++++>[<->-]>>+>[<[-]<
<+>>>-]>[-]+<<[>+>-<<-]<<<[>>+>+<<<-]>>>[<<<+>>>-]<>>[<+>-]<
<-[>[-]<[-]]>>+<[>[-]<-]<++++++++[<++++++<++++++>>-]>>>[>+>+
<<-]>>[<<+>>-]<[<<<<<.>>>>>-]<<<<<<.>>[-]>[-]++++[<++++++++>
-]<.>++++[<++++++++>-]<++.>+++++[<+++++++++>-]<.><+++++..---
-----.-------.>>[>>+>+<<<-]>>>[<<<+>>>-]<[<<<<++++++++++++++
.>>>>-]<<<<[-]>++++[<++++++++>-]<.>+++++++++[<+++++++++>-]<-
-.---------.>+++++++[<---------->-]<.>++++++[<+++++++++++>-]
<.+++..+++++++++++++.>++++++++[<---------->-]<--.>+++++++++[
<+++++++++>-]<--.-.>++++++++[<---------->-]<++.>++++++++[<++
++++++++>-]<++++.------------.---.>+++++++[<---------->-]<+.
>++++++++[<+++++++++++>-]<-.>++[<----------->-]<.+++++++++++
..>+++++++++[<---------->-]<-----.---.+++.---.[-]<<<]


[henrin]

-- Comments about reading the text file --

-1- The size is unknown: it may less or more than SOURCE_BUFFER_SIZE (10^5)
-2- fread() does not produce the end-of-string delimiter (null byte) that you check for end of read loop

On my side, here is what I use:

#include <io.h>

/*------------------------------------------------------------------------------*/
static char *afileLoad(char *fname)         /* loads a file in allocated text   */
/*--------------------------------------------------------------------------------
This function returns a pointer on the text buffer allocated for the file content.
This pointer can be used for further freeing. It is NULL in case of error.
--------------------------------------------------------------------------------*/
{
   char *buffer = NULL ;
   int h = _open(fname, 0) ;
   if (h)
   {
      long size = _filelength(h) ;
      if (size > 0)
      {
         buffer = calloc(size + 1, 1) ;      /* size + end null character   */
         if (buffer) _read(h, buffer, size) ;
      }
      _close(h) ;
   }
   return buffer ;
}

Note that calloc initialize buffer with null bytes. That way I can safely loop like this:

char c, *cp ;
for (cp = buffer ; c = *cp ; cp++)
{
    // c is the current character
    // cp points on the next one
}

One gets a warning on the test for end-of-loop:
    warning #2030: = used in a conditional expression.
It is not a mistake.

I hope this comments usefull.