3
\$\begingroup\$

Not a long time ago, I got into Rust and I made a simple Brainfuck Interpreter. Now I want to get back to Rust and I would like some comments on my code:

Just in case it's hard to read code from post:

https://github.com/DimChtz/brainfuck/tree/master/rust-brainpreter

lib.rs

mod core;

use core::parser::Parser;
use core::program::Program;
use core::instruction::Instruction;
use core::error::Error;
use core::memory::Memory;

use std::io;
use std::io::prelude::*;
use std::path::Path;

// Struct for the bf Interpreter (brainpreter)
pub struct Inter {

    tape:Memory,
    program:Program,
    parser:Parser,

}

impl Inter {

    // Function to create and return a new Interpreter
    pub fn new() -> Inter {

        Inter {

            tape: Memory::new(),
            program: Program::new(),
            parser: Parser::new(),

        }

    }

    // Function to load code into the parser (text)
    pub fn load<S: Into<String>>(&mut self, p:S) -> Result<(), Error> {

        self.parser.load(p)

    }

    // Function to load code into the parser (file)
    pub fn load_from_file<P: AsRef<Path>>(&mut self, src:P) -> Result<(), Error> {

        self.parser.load_from_file(src)

    }

    // Function to compile the code
    pub fn parse(&mut self) -> Result<(), Error> {

        // Call the parser
        match self.parser.parse() {

            Ok(p) => {

                self.program = p;

                Ok(())

            }

            Err(e) => Result::Err(e),

        }

    }

    // Function to execute bf program
    pub fn run(&mut self) -> Result<(), Error> {

        // Check if a program is properly loaded
        if self.program.get_size() == 0 {
            return Result::Err(Error::EmptyProgram);
        }

        // Execute instructions
        while let Some(instr) = self.program.get() {

            match instr {

                Instruction::IncPtr => { match self.inc_ptr() { Ok(_) => {  } Err(e) => return Result::Err(e), } }
                Instruction::DecPtr => { match self.dec_ptr() { Ok(_) => {  } Err(e) => return Result::Err(e), } }
                Instruction::IncVal => { match self.inc_val() { Ok(_) => {  } Err(e) => return Result::Err(e), } }
                Instruction::DecVal => { match self.dec_val() { Ok(_) => {  } Err(e) => return Result::Err(e), } }
                Instruction::Input  => { self.input(); }
                Instruction::Output => { self.output(); }
                Instruction::OpenBracket(n)  => { self.open_bracket(n); }
                Instruction::CloseBracket(n) => { self.close_bracket(n); }

            };

            // If this was the last instruction
            if !self.program.inc_ptr() {
                break;
            }

        };

        return Result::Ok(());

    }

    // Function to increase the value on the memory (tape)
    fn inc_val(&mut self) -> Result<(), Error> {

        self.tape.inc_val()

    }

    // Function to decrease the value on the memory (tape)
    fn dec_val(&mut self) -> Result<(), Error> {

        self.tape.dec_val()

    }

    // Function to increase the pointer on the memory (tape)
    fn inc_ptr(&mut self) -> Result<(), Error> {

        self.tape.inc_ptr()

    }

    // Function to decrease the pointer on the memory (tape)
    fn dec_ptr(&mut self) -> Result<(), Error> {

        self.tape.dec_ptr()

    }

    // Function to read into the memory (tape)
    fn input(&mut self) {

        match io::stdin().bytes().next() {

            Some(v) => self.tape.set_val(v.unwrap()),

            None => {  }

        }

    }

    // Function to print a char from memory (tape)
    fn output(&mut self) {

        print!("{}", self.tape.get_val() as char);

    }

    // Function to handle the open bracket
    fn open_bracket(&mut self, pos:usize) {

        if self.tape.get_val() == 0 {
            self.program.set_ptr(pos);
        }

    }

    // Function to handle the close bracket
    fn close_bracket(&mut self, pos:usize) {

        if self.tape.get_val() != 0 {
            self.program.set_ptr(pos);
        }

    }

}

core/mod.rs

pub mod instruction;
pub mod program;
pub mod parser;
pub mod error;
pub mod memory;

core/error.rs

use std::error;
use std::fmt;

#[derive(Debug)]
pub enum Error {

    NoSuchFile,
    EmptyProgram,
    MissingOpenBracket,
    MissingCloseBracket,
    PtrOverflow,
    PtrUnderflow,
    ValOverflow,
    ValUnderflow,

}

impl error::Error for Error {

    fn description(&self) -> &str {

        match *self {

            Error::NoSuchFile => "Failed to open file",
            Error::EmptyProgram => "The program is empty",
            Error::MissingOpenBracket => "Missing open bracket(s)",
            Error::MissingCloseBracket => "Missing close bracket(s)",
            Error::PtrOverflow => "Memory pointer overflow",
            Error::PtrUnderflow => "Memory pointer underflow",
            Error::ValOverflow => "Memory value overflow",
            Error::ValUnderflow => "Memory value underflow",

        }

    }

}

impl fmt::Display for Error {

    fn fmt(&self, f:&mut fmt::Formatter) -> fmt::Result {

        match *self {

            Error::NoSuchFile => write!(f, "Couldn't open file."),
            Error::EmptyProgram => write!(f, "The program is empty."),
            Error::MissingOpenBracket => write!(f, "Missing open bracket(s)."),
            Error::MissingCloseBracket => write!(f, "Missing close bracket(s)."),
            Error::PtrOverflow => write!(f,"Memory pointer overflow."),
            Error::PtrUnderflow => write!(f,"Memory pointer underflow."),
            Error::ValOverflow => write!(f,"Memory value overflow."),
            Error::ValUnderflow => write!(f,"Memory value underflow."),

        }

    }

}

core/instruction.rs

use std::fmt::{self, Display};

#[derive(Copy, Clone)]
pub enum Instruction {

    IncPtr,
    DecPtr,
    IncVal,
    DecVal,
    Input,
    Output,
    OpenBracket(usize),
    CloseBracket(usize),

}

impl Display for Instruction {

    fn fmt(&self, f:&mut fmt::Formatter) -> fmt::Result {

        match *self {

            Instruction::IncPtr => write!(f, ">"),
            Instruction::DecPtr => write!(f, "<"),
            Instruction::IncVal => write!(f, "+"),
            Instruction::DecVal => write!(f, "-"),
            Instruction::Input  => write!(f, ","),
            Instruction::Output => write!(f, "."),
            Instruction::OpenBracket(_) => write!(f, "["),
            Instruction::CloseBracket(_) => write!(f, "]"),

        }

    }

}

pub mod mapper {

    use super::Instruction;

    pub fn get_instr(c:char) -> Option<super::Instruction> {

        match c {

            '>' => Option::Some(Instruction::IncPtr),
            '<' => Option::Some(Instruction::DecPtr),
            '+' => Option::Some(Instruction::IncVal),
            '-' => Option::Some(Instruction::DecVal),
            ',' => Option::Some(Instruction::Input),
            '.' => Option::Some(Instruction::Output),
            '[' => Option::Some(Instruction::OpenBracket(0usize)),
            ']' => Option::Some(Instruction::CloseBracket(0usize)),
            _   => Option::None,

        }

    }

    #[allow(unused)]
    pub fn get_char(instr:Instruction) -> char {

        match instr {

            Instruction::IncPtr => '>',
            Instruction::DecPtr => '<',
            Instruction::IncVal => '+',
            Instruction::DecVal => '-',
            Instruction::Input => ',',
            Instruction::Output => '.',
            Instruction::OpenBracket(_) => '[',
            Instruction::CloseBracket(_) => ']',

        }

    }

}

core/memory.rs

use super::error::Error;

pub const MEMORY_SIZE:usize = 30000;

// Memory tape
#[derive(Debug)]
pub struct Memory {

    cells: Vec<u8>,
    ptr:usize,

}

impl Memory {

    // Function to create and return a new Memory tape
    pub fn new() -> Memory {

        let mut v = Vec::<u8>::new();
        v.push(0);

        Memory {

            cells: v,
            ptr:0,

        }

    }

    // Function to move the pointer to the right
    pub fn inc_ptr(&mut self) -> Result<(), Error> {

        self.ptr += 1;

        match self.ptr {

            n if n >= MEMORY_SIZE => {

                Result::Err(Error::PtrOverflow)

            }

            _ => {

                if self.ptr >= self.cells.len() {

                    self.cells.push(0);

                }

                Result::Ok(())

            }

        }

    }

    // Function to move the poiner to the left
    pub fn dec_ptr(&mut self) -> Result<(), Error> {

        match self.ptr {

            n if n == 0 => {

                Result::Err(Error::PtrUnderflow)

            }

            _ => {

                self.ptr -= 1;

                Result::Ok(())

            }

        }

    }

    // Function to increase the value
    pub fn inc_val(&mut self) -> Result<(), Error> {

        match self.cells[self.ptr].checked_add(1) {

            Some(v) => {

                self.cells[self.ptr] = v;

                Result::Ok(())

            }

            None => Result::Err(Error::ValOverflow),

        }

    }

    // Function to decrease the value
    pub fn dec_val(&mut self) -> Result<(), Error> {

        match self.cells[self.ptr].checked_sub(1) {

            Some(v) => {

                self.cells[self.ptr] = v;

                Result::Ok(())

            }

            None => Result::Err(Error::ValUnderflow),

        }

    }

    // Function to get the current value
    pub fn get_val(&self) -> u8 {

        self.cells[self.ptr]

    }

    // Function to set the current value
    pub fn set_val(&mut self, v:u8) {

        self.cells[self.ptr] = v;

    }

}

core/parser.rs

use super::instruction::{mapper, Instruction};
use super::program::Program;
use super::error::Error;

use std::io::prelude::*;
use std::fs::File;
use std::path::Path;
use std::convert::AsRef;

pub struct Parser {

    code_buffer:Vec<u8>,

}

impl Parser {

    // Function to create and return a new Parser
    pub fn new() -> Parser {

        Parser {
            code_buffer:Vec::<u8>::new(),
        }

    }

    // Function to load code from simple text
    pub fn load<S: Into<String>>(&mut self, buffer:S) -> Result<(), Error> {

        self.code_buffer = buffer.into().into_bytes();

        match self.code_buffer.len() {

            l if l > 0 => Result::Ok(()),

            _ => Result::Err(Error::EmptyProgram),

        }

    }

    // Function to load code from a file
    pub fn load_from_file<P: AsRef<Path>>(&mut self, src:P) -> Result<(), Error> {

        match File::open(src) {

            Ok(mut file) => {

                let mut content = String::new();

                match file.read_to_string(&mut content) {

                    _ => self.load(content)

                }

            }

            Err(_) => Result::Err(Error::NoSuchFile),

        }

    }

    // Function to compile the code into intermidiate code (program)
    pub fn parse(&mut self) -> Result<Program, Error> {

        // Create a new program
        let mut prog = Program::new();

        // Create a stack for the loop brackets
        let mut loop_stack = Vec::<usize>::new();

        // Create a code buffer ptr
        let mut code_ptr:usize = 0;

        // Compile the code buffer and feed the program
        if self.code_buffer.len() != 0 {

            loop {

                match self.code_buffer[code_ptr] as char {

                    b @ '+' => { prog.push(mapper::get_instr(b).unwrap()); }
                    b @ '-' => { prog.push(mapper::get_instr(b).unwrap()); }
                    b @ '>' => { prog.push(mapper::get_instr(b).unwrap()); }
                    b @ '<' => { prog.push(mapper::get_instr(b).unwrap()); }
                    b @ ',' => { prog.push(mapper::get_instr(b).unwrap()); }
                    b @ '.' => { prog.push(mapper::get_instr(b).unwrap()); }
                    b @ '[' => { 

                        // Add the instruction to the program (leave 0 for now)
                        prog.push(mapper::get_instr(b).unwrap());

                        // Update the loop stack
                        loop_stack.push(prog.get_size() - 1);

                     }
                    ']' => { 

                        if loop_stack.is_empty() {

                            // Missmatch `]` bracket found
                            return Result::Err(Error::MissingOpenBracket);

                        }

                        let curr_ptr = prog.get_size();

                        let last_open_bracket_ptr = loop_stack.pop().unwrap();

                        prog.update_instr(last_open_bracket_ptr, Instruction::OpenBracket(curr_ptr));

                        prog.push(Instruction::CloseBracket(last_open_bracket_ptr));

                     }
                    _   => { /* IGNORE UNUSED SYMBOLS */ }

                };

                code_ptr += 1;

                if code_ptr >= self.code_buffer.len() {
                    break;
                }

            }

            // Check if loop_stack is empty or not -> not = error (missmatched brackets)
            if !loop_stack.is_empty() {
                return Result::Err(Error::MissingCloseBracket);
            }

        } else {

            // The code buffer is empty -> empty code loaded or just never loaded
            return Result::Err(Error::EmptyProgram);

        }

        // Program is empty return Error (code buffer contains only unused symbols)
        if prog.get_size() == 0 {
            return Result::Err(Error::EmptyProgram);
        }

        return Result::Ok(prog);

    }

}

core/program.rs

use super::instruction::Instruction;

// Struct for the program (a series of instructions)
pub struct Program {

    instr:Vec<Instruction>,
    ptr:usize,

}

impl Program {

    // Function to create and return a new program
    pub fn new() -> Program {

        Program {

            instr:Vec::<Instruction>::new(),
            ptr:0,

        }

    }

    // Function to increase the pointer
    pub fn inc_ptr(&mut self) -> bool {

        if self.ptr < self.instr.len() - 1 {
            self.ptr += 1;
            return true;
        }

        false

    }

    // Function to decrease the pointer
    #[allow(unused)]
    pub fn dec_ptr(&mut self) -> bool {

        if self.ptr > 0 {
            self.ptr -= 1;
            return true;
        }

        false

    }

    // Function to move the pointer
    pub fn set_ptr(&mut self, pos:usize) -> bool {

        if pos >= self.get_size() {
            return false;
        }

        self.ptr = pos;

        true

    }

    // Function to get the current instruction (if a program is already loaded and the instruction exists)
    pub fn get(&self) -> Option<Instruction> {

        if self.instr.len() > 0 { Option::Some(self.instr[self.ptr]) } else { Option::None }

    }

    // Function to increase the pointer and get the instruction
    #[allow(unused)]
    pub fn get_next(&mut self) -> Option<Instruction> {

        if self.inc_ptr() { self.get() } else { Option::None }

    }

    // Function to decrease the pointer and get the instruction
    #[allow(unused)]
    pub fn get_prev(&mut self) -> Option<Instruction> {

        if self.dec_ptr() { self.get() } else { Option::None }

    }

    // Function to get an instruction
    #[allow(unused)]
    pub fn get_at(&self, p:usize) -> Option<Instruction> {

        if p >= (0 as usize) && p < self.instr.len() { Option::Some(self.instr[p]) } else { Option::None }

    }

    // Function to add a new instruction at the end of the program
    pub fn push(&mut self, i:Instruction) -> &mut Program {

        self.instr.push(i);

        self

    }

    // Function to remove the last instruction from the program (if exists)
    #[allow(unused)]
    pub fn pop(&mut self) -> bool {

        return match self.instr.pop() {

            Some(_) => {

                if self.ptr >= self.instr.len() {
                    if self.ptr > 0 {
                        self.ptr -= 1;
                    }
                }

                true

            }

            None => false

        }

    }

    // Function to clear the program and set pointer to 0
    #[allow(unused)]
    pub fn clear(&mut self) -> &mut Program {

        self.instr.clear();
        self.ptr = 0;

        self

    }

    // Function to reset the pointer (set to 0)
    #[allow(unused)]
    pub fn reset_ptr(&mut self) -> &mut Program {

        self.ptr = 0;

        self

    }

    // Function to update a specific instruction
    pub fn update_instr(&mut self, pos:usize, new_instr:Instruction) -> bool {

        if self.instr.len() == 0 || pos >= self.instr.len() {
            return false;
        }

        self.instr[pos] = new_instr;

        return true;

    }

    // Function to get program's size
    pub fn get_size(&self) -> usize {

        self.instr.len()

    }

}
\$\endgroup\$
  • \$\begingroup\$ Do you have a repository for this? It is correct, that you post the code here, but it's a bit cumbersome to scrap the code from this post. \$\endgroup\$ – hellow Feb 5 at 8:32
  • \$\begingroup\$ @hellow Sorry, should I delete this? Yes, I added a link the repository in case it's hard to read the code from the post. \$\endgroup\$ – DimChtz Feb 5 at 8:35
  • \$\begingroup\$ No! As said, it is the correct way. Always post the related to to stackexchange, so it does not rely on external sources. But it's a extra step so it is easier for me to get and inspect the code, instead of doing copy&paste 8 times and setting up the repo correctly. \$\endgroup\$ – hellow Feb 5 at 8:36
  • \$\begingroup\$ Wow.. I'm actually stupid. You already did provide the link to the repo in the original post, but I thought it would go to wikipedia or something similar which explains what brainfuck actually is. Sorry for the noise :| \$\endgroup\$ – hellow Feb 5 at 8:52
  • \$\begingroup\$ @hellow No worries. My bad, I would think it's a link to wikipedia as well lol \$\endgroup\$ – DimChtz Feb 5 at 8:56
0
\$\begingroup\$

(Please note: that I'm writing this while I walk through your code, so things I spot I will write down. This so can be sometimes a bit difficult to follow, I apologize!)

Rust has pretty good tooling, e.g. rustfmt which ensures, that the code you wrote compiles with the rust style guidlines. This is the first step you should do (it's pure formatting, does nothing else).

Second, there is clippy which spots common mistakes in your code, e.g. using return 3; as a last statement in a function (which is not recommended, but not actually wrong).

The neat thing is, that this is really painless to install and use.

  • rustup component add clippy rustfmt
    • installs clippy and rustfmt for your current toolchain
  • cargo rustfmt
    • formats the code, you may want to commit the code before processing further
  • cargo clippy
    • Spots common mistakes and potential errors in your code

To highlight some of the things you do which are not considered as rusty:


   --> src/core/parser.rs:129:9
129 |         return Result::Ok(prog);

Make this

Ok(prog)

You can omit return and espcially Result, because it is in the prelude of std. Also do this for other occurences, e.g. src/core/program.rs:139 and src/lib.rs:100. Especially you do Result::Ok/Err very often, although you can shorten it to Ok(...) or Err(...).


Transform

src/core/parser.rs:60:12
60 |         if self.code_buffer.len() != 0 {

to

if !self.code_buffer.is_empty() {

src/lib.rs:125:9
    |
125 | match io::stdin().bytes().next() {
126 |     Some(v) => self.tape.set_val(v.unwrap()),
127 | 
128 |     None => {}
129 | }
if let Some(v) = io::stdin().bytes().next() {
    self.tape.set_val(v.unwrap());
}

You use unwrap quite often. You should either consider using a Result or {Option,Result}::expect which will panic with a custom message instead of the default one which you can use to explain what went wrong and why. It is considered to be better than using plain unwrap (unless you really, really know it cannot fail, but often you should use a if let then like I showed above.


I would recommend using #[derive(Debug)] on types, so debugging is easier. Also provide a Default implementation where suiteable, e.g. for Inter, Memory, Program and Parser. A rule of thumb is, whenever you have a new "constructor" with no arguments, provide a Default impl.

#[derive(Debug, Default)]
pub struct Inter {
    tape: Memory,
    program: Program,
    parser: Parser,
}

I would rewrite the Inter::load and Inter::load_from_file as static methods. In contrast to other OOP-languages rust does not have an explicit constructor and is therefore not bound to a specific return value, so you can have something like fn new() -> Result<Self, Error> which is really neat. Or you could provide a method with_parser to Inter, e.g.

pub fn with_parser(parser: Parser) -> Self {
    Inter {
        parser,
        ..Default::default()
    }
}

This way you would get rid of redudant methods, which is always a good thing!

Parser::load would then look similar to

impl Parser {
    pub fn load<S: Into<String>>(buffer: S) -> Result<Self, Error> {
        let buffer = buffer.into().into_bytes();
        if buffer.is_empty() {
            Err(Error::EmptyProgram)?; // or return Err(Error::EmptyProgram);
        }
        Ok(Parser {
            code_buffer: buffer.into_bytes()
        })
    }
}

Same goes for load_from_file.


Your Inter does not new the parser member at all. You could get rid of it completly, because you only use it for parsing text.
So instead of providing a new function without parameters on Inter, I would have a function: pub fn with_program(program: Program) -> Self which you can call like

fn run() -> Result<(), Error> {
    let interpreter = Inter::with_program(Parser::load_from_file("/dev/urandom")?.parse()?);
    // ...
}

This of course makes it impossible implementing Default for Inter, unless you want it to be possible to create an empty tape/program (You also could make Parser::load_from_file return a Result<Program, Error> instead, so you get rid of the extra parse call.


Why do you parse bytes, instead of chars? You suspect, that you are only getting ascii chars, but what if there is a ß or Õ in the text you load. This will lead to very weird errors.
Also the function looks very c-ish. You have a lot of mutable variables and index variables. Rust doesn't do that, instead just go with (self.input is a String): for c in self.input.chars().
Also you can group common actions together, e.g.

for c in self.code.chars() {
    match c {
        '+' | '-' | '>' | ... /* insert rest here */ => { unimplemented!() },
        _ => { /* the rest */ unimplemented!() },
    }
}

These are just a few things, I do not have time (sorry!), but I think you can start with this. If you want I can update it later on with more examples and how to do it differently.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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