patterncppMinor
Toy virtual machine
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
vm/vm.cpp
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
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_Hvm/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 you're not planning to modify the parameter
-
Using (
Here, every
-
Try to declare your variables only when you need them. In
-
Your destructor isn't doing anything in
-
This is something we shouldn't see anymore:
C++11 includes the C99 header `
-
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.