2
\$\begingroup\$

I am currently implementing a virtual machine that runs a programming language similar to assembly. The difference is that an instruction in this language can have any number of operands. and the machine should not give an error. whatever the operand is inside the memory, the machine should make sense out of it. If the machine only has four registers (r0-r3) and an operand is r5 then the operand will be referring to register 1 (5 % 4 = 1). the operand has two addressing modes only. register 'r' and array register 'a'

Example:

add r5       // r1 += 2
add          //r0 += 2
add a0, r5   //for all i in array_register[0] .. a[i] += r1

The virtual machine has 4 registers (defined as floats)

std::array<float, 4> registers;

and 4 array registers. The array register contains an array of size defined as MAX_ARRAY_SIZE (the virtual machine's hardware array size) and an integer that states the size of the array assigned to that array register.

typedef std::pair<int, std::array<float, MAX_ARRAY_SIZE>> array_data;    
std::array<array_data, 4> array_registers;

The memory is defined:

std::vector<Instruction> translated_memory;

An Instruction holds a label, an operator and a vector of operands

/*struct that holds an instruction with its operands in a translated memory*/
struct Instruction
{
    Operator op;
    int label;
    Operands operands;

}; 

Operators are defined in an enum

/*enum that holds all the instructions set*/

    enum Operator { 
        //only 4 for simplicity of the question
        //MATHEMATICAL INSTRUCTIONS
        ADD, SUB, MUL, DIV 
     };

An operand is defined as pair of bool (true for array addressing mode and false for normal register addressing mode) and a float that has the value of the operand.

Operands (used in Instruction struct) is vector of operands

typedef std::vector<std::pair<bool, float >> Operands;

For every operator there is a c++ function.

The function of any operator has the following definition:

typedef void(*FunctionOfInstruction)(Operands);

A map is used to map the operator enum with the its function:

std::map<Operator, FunctionOfInstruction> instruction_functions;

Ahe add operator for example, has a function

void _ADD(Operands operands);

and is mapped in the map instruction_functions as follows:

instruction_functions[Operator::ADD] = &_ADD;

The VM runs, after loading the memory; translated_memory

void run_vm()
{
    //std::cout << "inside run \n";
    if (translated_memory.empty())  return;

    pc = 0;
    while (true)
    {
        if (pc > translated_memory.size() - 1)  return;
              instruction_functions[translated_memory.at(pc).op(translated_memory.at(pc).operands);
    }
}

My problem is in the implementation of the functions being mapped to operators. this is how the _SUB function looks like:

void _SUB(Operands operands)
{
    if (operands.size() == 0)
    {
        //SUB .. r0--
        registers[0]--;
    }
    else if (operands.size() == 1)
    {
        if (operands[0].first == IS_ARRAY)
        {
            //SUB aX    .. aX[i]-- for all used i in aX
            for (auto it = array_registers[(int)operands[0].second % 4].second.begin(); 
                    it != array_registers[(int)operands[0].second % 4].second.begin() 
                    + array_registers[(int)operands[0].second % 4].first; ++it)

                *it--;

        }
        else
        {
            //SUB rX    .. rX--
            registers[(int)operands[0].second % 4]--;
        }
    }
    else
    {
        if (operands[0].first == IS_ARRAY && operands[1].first == IS_ARRAY)
        {
            //SUB aX, aY    .. aX[i]-=aY[i]   for all used i in aX and aY
            for (auto i = 0; (i < array_registers[(int)operands[0].second % 4].first 
                    && i < array_registers[(int)operands[1].second % 4].first); i++)

                array_registers[(int)operands[0].second % 4].second[i] -=
                array_registers[(int)operands[1].second % 4].second[i];

        }
        else if (operands[0].first == IS_ARRAY && operands[1].first == !IS_ARRAY)
        {
            //SUB aX, rY    .. aX[i]+=rY    for all used i in aX    
            for (auto it = array_registers[(int)operands[0].second % 4].second.begin(); 
                    it != array_registers[(int)operands[0].second % 4].second.begin() 
                    + array_registers[(int)operands[0].second % 4].first; ++it)

                *it -= registers[(int)operands[1].second % 4];

        }
        else if (operands[0].first == !IS_ARRAY && operands[1].first == IS_ARRAY)
        {
            //SUB rX, aY    .. rX-=sizeOf(aY[])     size used of aY and not maximum size of the register
            registers[(int)operands[0].second % 4] -= 
            array_registers[(int)operands[1].second % 4].first;
        }
        else
        {
            //SUB rX, rY    .. rX-=rY;
            registers[(int)operands[0].second % 4] -=
            registers[(int)operands[1].second % 4];
        }

    }
    pc++;   //increment pc
}

the lines are too long and the data structure is unclear by just reading it.

I am trying to use macros to make things clearer and easier to read, but I am not sure if this is a good and proper way of using macros.

for getting the registers values and array register's size (this part I think is okay)

#define REGISTER_ARRAY_SIZE_AT(index)           array_registers[index % 4].first
#define REGISTER_ARRAY_AT(index)                array_registers[index % 4].second

#define REGISTER_ARRAY_BEGIN_AT(index)          REGISTER_ARRAY_AT(index % 4).begin()
#define REGISTER_ARRAY_END_AT(index)            REGISTER_ARRAY_AT(index % 4).begin() + REGISTER_ARRAY_SIZE_AT(index % 4)

#define REGISTER_AT(index)                      registers[index % 4]

the following macros I believe are wrong to have. every function has a parameter Operands operands in the macros I am using operands which is a local variable inside every operator's function

#define NUMBER_OF_OPERANDS                      operands.size()
#define OPERAND_ADDRESSING_MODE_AT(index)       (int) operands[index].first
#define OPERAND_VALUE_AT(index)                 (int) operands[index].second

so I'll be able to write something like this:

if (NUMBER_OF_OPERANDS == 0)
    {
        //ADD .. r0++
        REGISTER_AT(0)++;
    }
    else if (operands.size() == 1)
    {
        //if (operands[0].first == CRAB_IS_ARRAY)
        if (OPERAND_ADDRESSING_MODE_AT(0) == IS_ARRAY)
        {
            //ADD aX    .. aX[i]++ for all used i in aX
            for (auto it = REGISTER_ARRAY_BEGIN_AT( OPERAND_VALUE_AT(0) );
                    it != REGISTER_ARRAY_END_AT( OPERAND_VALUE_AT(0) ); ++it)

                *it++;

        }
        else
        {
            //ADD rX    .. rX++
            REGISTER_AT(OPERAND_VALUE_AT(0))++;
        }
    }

How good/bad is this? Any suggestions to have a proper way to make the code cleaner and more readable?

\$\endgroup\$

migrated from stackoverflow.com Oct 13 '16 at 19:00

This question came from our site for professional and enthusiast programmers.

  • 8
    \$\begingroup\$ Using macros is typically not the way to make code clean. \$\endgroup\$ – Jonathan Leffler Oct 1 '16 at 2:14
  • 1
    \$\begingroup\$ I don't see how NUMBER_OF_OPERANDS is clearer than operands.size() tbh. You are hiding some of the logic behind macros and it makes the code difficult to reason on (because you see only an abstraction) and hard to maintain later on when you come to make a change or fix a bug. \$\endgroup\$ – Galik Oct 1 '16 at 2:53
  • \$\begingroup\$ @Galik yes operands.size() is easier to read but my concern was with something like for (auto it = array_registers[(int)operands[0].second % 4].second.begin(); it != array_registers[(int)operands[0].second % 4].second.begin() + array_registers[(int)operands[0].second % 4].first; ++it) is this okay? I've never had a work experience in software and I'm trying to practice having clean code \$\endgroup\$ – T.J. Oct 1 '16 at 2:59
  • \$\begingroup\$ If you want to make lines like that easier to understand then you can always break them down into simpler steps assigning intermediate parts to temporary variables. I just don't think macros is a good tool for this. I would prefer to break the code into smaller expressions and functions. I suspect having to cross-reference macro definitions all the time would be harder to follow. \$\endgroup\$ – Galik Oct 1 '16 at 3:11
  • \$\begingroup\$ @Galik thanks, I obviously have a wrong understanding of macros. I'll look into macros more in details to understand how it is used. \$\endgroup\$ – T.J. Oct 1 '16 at 3:16

Your Answer

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