First project: A brainf** interpreter

Hello everyone! This is my first post here.

I just completed my first rust project: a Brainf** interpreter (for those of you who are not aware, Brainf** is a minimalist programming language)

After coding, I ran cargo fmt to format my code and also ran a linter check with cargo clippy. The project builds with no compiler or linter warnings. Is there anything else I need to keep in mind?

I would like you to review my code and also offer you kind suggestions.

Thank you!

First thing I noticed: you're using char (which takes 32 bits in memory) to store instructions (which carry only 3 bits of information).

I would suggest making an enum of the instructions. Then you'll use less RAM, and the processing match can be exhaustively checked to be sure you've implemented all of them -- and thus you'd be able to remove this vestigial arm.

You might do that by changing this filter to a flat_map.


This is a classic Parse, don’t validate suggestion -- rather than checking that you only have the values you understand, and assuming in a bunch of processing code later, convert the input into something that can only represent what you expect, so your brain doesn't need to remember implicit context.

4 Likes

A couple of things, not in order of importance:

  • Read and adhere to the Rust API Guidelines Checklist. For example, you are not implementing any common traits (Default, Clone, Debug) on your structs. The entire Machine::new() function could be removed and replaced with the automatically derived implementation of Default.
  • The execution of loop instructions seems to be more convoluted than it needs to be. Perhaps because when you find an opening bracket, you initialize the bracket level to 0 (rather than 1), so you've got an extra condition to check. I would try to clean up these instructions. You are not really parsing the instructions in parse_instructions, either – you are effectively re-parsing the structure of brackets every time you get to one.
  • The following piece and its analogous repetitions are unnecessary:
    self.data_tape[self.data_pointer] = if self.data_tape[self.data_pointer] == 255
    {
        0
    } else {
        self.data_tape[self.data_pointer] + 1
    }
    
    This is simply u8::wrapping_add (and u8::wrapping_sub, etc.).
  • In main(), it's wasteful to collect() all arguments into a vector if you are only going to use the first item anyway. Use next() or nth(0) instead.
3 Likes

Or even Wrapping::<u8>::add etc., if non-wrapping operations are not needed.

1 Like

Thank you @scottmcm @H2CO3 and Cerber-Ursi for taking your time and giving feedback. I spent the entire day making changes to my code incorporating your suggestions.

  1. I am now using an enum for encoding and parsing the instructions.

  2. I was not aware of the Rust API Guidelines. I have now implemented some of the common interoperability traits for the Machine struct. I was also able to replace Machine::new() with Machine::default()

  3. I have tried to simplify the execution of the instructions as much as I could by getting rid of the unnecessary checks and statements and making use of standard library functions like u8::wrapping_add, u8::wrapping_sub and u8::rem_euclid.

  4. Changing collect() in main() to nth(1) was simple enough

After all of this the code still compiles with no warnings and the linter gives no warnings as well.

Are the changes I made all right? what other changes do I need to make?

It's great to see more traits. You can save yourself a bunch of work by deriving them, though.

For example, these are what the derive would do:

so you can replace those with just

-#[derive(Clone, Hash)]
+#[derive(Clone, Hash, Debug, Default)]
 pub struct Machine {
     instr_tape: Vec<Instr>,
     data: [u8; TAPE_LEN],
     pointer: usize,
 }

Also related to the derives, it's odd to see Hash without Eq, because the normal place to make use of Hash is in HashMap in std::collections - Rust, where the key type wants both Hash & Eq -- because just Hash isn't enough to lookup something in a hashmap, due to the collision potential.

So you probably want to add it in places like

-#[derive(Clone, Hash, Debug, PartialEq)]
+#[derive(Clone, Hash, Debug, PartialEq, Eq)]
 enum Instr {

As more stylistic feedback, this is a bit repetitive and imperative:

It's not incorrect at all, but you might consider trying out doing more by iterator adapters. Perhaps something like

    pub fn parse_instructions(&mut self, contents: String) {
        self.instr_tape.extend(contents.chars().flat_map(|c| {
            Some(match c {
                '+' => Instr::IncrDataByte,
                '-' => Instr::DecrDataByte,
                '>' => Instr::IncrDataPointer,
                '<' => Instr::DecrDataPointer,
                '.' => Instr::Print,
                ',' => Instr::Input,
                '[' => Instr::JumpForward,
                ']' => Instr::JumpBackward,
                _ => return None,
            })
        }));
    }
2 Likes