Refactor: refactor + tests for opcode enum

[nerodesu017]

Pull Request Overview

This pull request refactors so that an enum is used for opcodes instead of their raw byte representation we use constant values in order to improve the readability of the code. This pull request also adds test cases for the above implementation.

Also, the usage of panic is correct, as we would exit out at the validation point when an instruction we haven't implemented yet is encountered. It is also used for correctly assessing the outputs of the tests.

Testing Strategy

This pull request was tested locally using the test cases provided.

TODO or Help Wanted

This pull request still needs an opinion if it is the right approach.

Formatting

• Ran cargo fmt
• Ran cargo check
• Ran cargo build

Github Issue

N/A

Author

Signed-off-by: Popescu Adrian adrian.popescu@oxidos.io

[wucke13]

Here is another Idea I quickly stitched together; we should also get @florianhartung feedback on that as he dismissed this idea earlier, IIRC.

mod opcodes {
    const NOP: u8 = 0x01;
    const End: u8 = 0x0B;
    const LocalGet: u8 = 0x20;
    const LocalSet: u8 = 0x21;
    const GlobalGet: u8 = 0x23;
    const GlobalSet: u8 = 0x24;
    const I32Load: u8 = 0x28;
    const I32Store: u8 = 0x36;
    const I32Const: u8 = 0x41;
    const I32Add: u8 = 0x6A;
    const I32Mul: u8 = 0x6C;
    const FCInstructions: u8 = 0xF;
}

mod fc_opcodes {
    const I32TruncSatF32s: u8 = 0x00;
}

struct Interpreter<'a> {
    bytecode: &'a [u8],
    pc: usize,
    stack: Vec<u8>,
}

impl<'a> Interpreter<'a> {
    fn run(&mut self) -> Result<(), Box<dyn std::error::Error>> {
        use fc_opcodes::*;
        use opcodes::*;

        let Self {
            pc,
            bytecode,
            stack,
        } = self;

        match &bytecode[*pc..] {
            [NOP, ..] => {
                *pc += 1;
            }
            [LocalSet, x, ..] => {
                stack.push(*x);
                *pc += 2;
            }
            [FCInstructions, I32TruncSatF32s, ..] => {
                // do some work
                *pc += 2;
            }
            _ => {
                todo!("something left to be implemented");
            }
        }

        Ok(())
    }
}

[nerodesu017]

Interesting way of doing this, I'd refactor it this way if Florian and Alex agree.

[valexandru]

Looks clean to me what you propose @wucke13, the only downside that I can find would be the refactoring of the code for this.

[wucke13]

Thank you for the comments! There is two downsides to my way, it contains a variable amount of decisions (possibly hindering optimization by the Compiler), and the const stuff is rather elaborate. Let's wait for @florianhartung feedback before we continue this approach.

[florianhartung]

LGTM