HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Toy virtual machine

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
toyvirtualmachine

Problem

I am a web developer, trying to have fun with C++11, so any positive/negative feedback is very welcome.

This is a toy virtual machine with 3 registers and 3 commands:

vm/vm.h

#ifndef VM_H
#define VM_H

#include 
#include 

#include "vtypes.h"

namespace violet
{

    class Vm
    {
        bool running;
        std::vector program;
        u16  regs[REGS_N];
        u16  pc;

    public:
        Vm(const std::vector);
        ~Vm();

        u16 fetch();
        int run();
        std::vector dump_regs() const;
        instruction decode(const unsigned int) const;
    };

}

#endif // VM_H


vm/vm.cpp

#include "vm.h"

namespace violet
{

    Vm::Vm(const std::vector program):
        running(false),
        program(program),
        pc(0)
    {

    }

    Vm::~Vm()
    {

    }

    u16 Vm::fetch()
    {
        return program.at(pc++);
    }

    int Vm::run()
    {
        running = true;
        instruction instr;

        if (program.empty()) return 1;
        while(running && pc  Vm::dump_regs() const
    {
        return std::vector(std::begin(regs), std::end(regs));
    }

    instruction Vm::decode(const unsigned int opcode) const
    {
        instruction i;
        // Bits 15-12  (4bits) Instruction number
        // Bits 11-8   (4bits) Register number
        // Bits 7-0    (8bits) Immediate value

        i.code = (opcode & 0xF000) >> 12;
        i.r0   = (opcode & 0xF00)  >>  8;
        i.r1   = (opcode & 0xF0)   >>  4;
        i.r2   = (opcode & 0xF);
        i.imm  = (opcode & 0xFF);

        return i;
    }

}


vm/vtypes.h

```
#ifndef VTYPES_H
#define VTYPES_H

using u8 = unsigned char; // At least 8-bits
using u16 = unsigned short; // At least 16-bits - (Usually)

namespace violet
{

const unsigned int REGS_N {3};

// Instructions
const unsigned int i_halt {0};
const unsigned int i_loadi {0x1};
const unsigned int i_add {0x2};

struct instruction
{
unsigned int code;
unsign

Solution

A few notes to complete what @vnp already said:

-
When you open several namespaces at once and close them at once, you better indent only once since the double indentation won't add to clarity but will produce "longer" lines. A future revision of the standard may introduce nested namespaces definition, which makes clear that two namespaces opened and closed at once are generally considered a a single code unit.

-
if (out.fail()) is fine, but the idiomatic way to check whether errors occured while opening your file would be if (not out). It's true for input fstreams as well as for output fstreams.

-
If you're not planning to modify the parameter filename in write_program_file then you might as well take it by const reference instead of taking it by value. That also applies to program and most of the function parameters in your program (and in the wild).

-
Using (const) references may also improve this loop:

for (auto line : source) { /* ... */ }


Here, every line is a copy of its equivalent in source. It doesn't seem that you alter line afterwise, therefore, you could make it a const reference instead:

for (const auto& line : source) { /* ... */ }


-
Try to declare your variables only when you need them. In load_source_file, you better declare line and result only after the error check, otherwise you will end up default-initializing them even when there is an error and your program will waste some time for construction and destruction. That may not really impact the speed of your program specifically, but on a more general note, try not to perform useless operations.

-
Your destructor isn't doing anything in Vm, so you better follow the rule of zero and not implement it.

-
This is something we shouldn't see anymore:

using u8  = unsigned char;    // At least 8-bits
using u16 = unsigned short;   // At least 16-bits   -  (Usually)


C++11 includes the C99 header ` (under the name ) where typedefs such as uint8_t and uint16_t plus many more variants are defined. If you want a fixed-type integer, use , do not define it yourself.

-
You could use typed
enum to represent your instructions instead of several const unsigned int variables:

// Instructions
enum instructions: unsigned int
{
    i_halt  = 0x0,
    i_loadi = 0x1,
    i_add   = 0x2
};


You can even use a scoped enumeration (
enum class) if you want more type safety.

-
Instead of
u16 regs[REGS_N];, you could use an std::array. std::array is a mere wrapper around a C array and its simplicity makes it generally zero-overhead. Moreover, it offers container-like goodies such as iterators. It's a more secure class for free! :)

-
Why do you take a
const char* parameter in the function die while you use std::string everywhere else in the project? You might as well be consistent and use an std::string` here too.

Code Snippets

for (auto line : source) { /* ... */ }
for (const auto& line : source) { /* ... */ }
using u8  = unsigned char;    // At least 8-bits
using u16 = unsigned short;   // At least 16-bits   -  (Usually)
// Instructions
enum instructions: unsigned int
{
    i_halt  = 0x0,
    i_loadi = 0x1,
    i_add   = 0x2
};

Context

StackExchange Code Review Q#82602, answer score: 9

Revisions (0)

No revisions yet.