10
\$\begingroup\$

Simple Machine Translator(SML) is a simulator that executes code written in hexadecimal. It supports features such as read, write, add, subtract and many more. My previous question concerning this minimal exercise link can be found here for those who wants to follow up. I made a lot of changes, restructured and move things around and would appreciate a review.

SML.h

#ifndef SML_SML_H_
#define SML_SML_H_

#include "evaluator.h"

#include <string>

constexpr size_word register_max_size = 6;

enum REGISTERS
{
    ACCUMULATOR = 0, 
    INSTRUCTION_COUNTER = 1,
    TEMPORARY_COUNTER = 2,
    INSTRUCTION_REGISTER = 3,
    OPERATION_CODE = 4,
    OPERAND = 5
};

class SML 
{   
    friend void swap( SML &lhs, SML &rhs );
    friend class Evaluator;

    public:
        SML() = default;
        SML( const int memory_size, const int word_lower_lim, const int word_upper_lim );
        SML( const SML &s );
        const SML& operator=( const SML s );
        SML( SML &&s );
        ~SML();

        void display_welcome_message() const;
        void load_program();
        void execute();
        
    private:
        size_word registers[ register_max_size ];
        std::string temp_str; // holds the string before it is written into the memory
        bool debug;

        static const size_word read_ = 0xA; // Read a word(int) from the keyboard into a specific location in memory
        static const size_word write_ = 0xB; // Write a word(int) from a specific location in memory to the screen
        static const size_word read_str_ = 0xC; // Read a word(string) from the keyboard into a specific location in memory
        static const size_word write_str_ = 0xD; // Write a word(string) from a specific location in memory to the screen
        static const size_word load_ = 0x14; // Load a word from a specific location in memory to the accumulator
        static const size_word store_ = 0x15; // Store a word from the accumulator into a specific location in memory
        static const size_word add_ = 0x1E; /* Add a word from a specific location in memory to the word in the accumulator; store the 
                                        result in the accumulator */
        static const size_word subtract_ = 0x1F;
        static const size_word multiply_ = 0x20;
        static const size_word divide_ = 0x21;
        static const size_word modulo_ = 0x22;
        static const size_word branch_ = 0x28; // Branch to a specific location in the memory
        static const size_word branchneg_ = 0x29; // Branch if accumulator is negative
        static const size_word branchzero_ = 0x2A; // Branch if accumulator is zero
        static const size_word halt_ = 0x2B; // Halt the program when a task is completed
        static const size_word newline_ = 0x32; // Insert a new line
        static const size_word end_ = -0x1869F; // End the program execution
        static const size_word sml_debug_ = 0x2C; // SML debug ( 1 to turn on, 0 to turn off )
        size_word word_lower_limit; /* A word should not exceed */
        size_word word_upper_limit;  /*    this limits */

        size_word memory_size;
        size_word *memory = nullptr;
        
        void set_registers();
        void memory_dump() const;
};

#endif

SML.cpp

#include "sml.h"

#include "evaluator.h"

#include <iostream>
#include <iomanip>
#include <algorithm>

SML::SML( const int mem_size, const int word_lower_lim, const int word_upper_lim ) 
: debug( false ), word_lower_limit( word_lower_lim ),
word_upper_limit( word_upper_lim ), memory_size( mem_size )
{
    set_registers();
    memory = new size_word[ memory_size ];    
}

void SML::set_registers()
{
    registers[ static_cast<unsigned>( ACCUMULATOR ) ] = 0;
    registers[ static_cast<unsigned>( INSTRUCTION_COUNTER ) ] = 0;
    registers[ static_cast<unsigned>( TEMPORARY_COUNTER ) ] = 0;
    registers[ static_cast<unsigned>( INSTRUCTION_REGISTER ) ] = 0;
    registers[ static_cast<unsigned>( OPERATION_CODE ) ] = 0;
    registers[ static_cast<unsigned>( OPERAND ) ] = 0;
}


SML::SML( const SML &s )
{
    temp_str = s.temp_str;
    debug = s.debug;
    word_lower_limit = s.word_lower_limit;
    word_upper_limit = s.word_upper_limit;
    std::copy( std::cbegin( s.registers ), std::cend( s.registers ), registers );
    memory_size = s.memory_size;
    memory = new size_word[ memory_size ];
    std::copy( s.memory, s.memory + s.memory_size, memory );
}

SML::SML( SML &&s )
{
    swap( *this, s );
    memory = new size_word[ memory_size ];
    std::move( s.memory, s.memory + s.memory_size, memory );
}

const SML& SML::operator=( SML s )
{
    swap( *this, s );
    memory = new size_word[ memory_size ];
    std::move( s.memory, s.memory + s.memory_size, memory );

    return *this;
}

void swap( SML &lhs, SML &rhs ) 
{
    using std::swap;

    swap( lhs.temp_str, rhs.temp_str );
    swap( lhs.debug, rhs.debug );
    swap( lhs.word_lower_limit, rhs.word_lower_limit );
    swap( lhs.word_upper_limit, rhs.word_upper_limit );
    swap( lhs.memory_size, rhs.memory_size );
    swap( lhs.registers, rhs.registers );
}

void SML::display_welcome_message() const 
{
    std::cout << "***" <<  " WELCOME TO SIMPLETRON! " << "***\n\n";
    std::cout <<  std::setw( 5 ) << std::left << "***" 
        << "Please enter your program one instruction" 
        << std::setw( 5 ) << std::right << "***\n";

    std::cout << std::setw( 5 ) << std::left  << "***" 
    << "(or data word) at a time. I will type the" 
    << std::setw( 5 ) << std::right << "***\n"; 

    std::cout << std::setw( 5 ) << std::left << "***" 
    << "location number and a question mark (?)."  
    << std::setw( 6 ) << std::right << "***\n";

    std::cout << std::setw( 5 )  << std::left << "***"
    << "You then type the word for that location"  
    << std::setw( 6 ) << std::right  << "***\n";

    std::cout << std::setw( 5 )  << std::left << "***" 
    << "Type the sentinel -0x1869F to stop entering"  
    << std::setw( 5 ) << std::right << "***\n";

    std::cout << std::setw( 5 )  << std::left << "***" 
    << "your program"  
    << std::setw( 5 ) << std::right << "***";

    std::cout << "\n\n" << std::flush;
}

void SML::load_program() 
{
    size_word &ins_cnt = registers[ static_cast<unsigned>( INSTRUCTION_COUNTER ) ];
    size_word temp;
    while( ins_cnt != memory_size ) 
    {
        std::cout << std::setw( 2 ) << std::setfill( '0' )
            << ins_cnt << " ? ";
        std::cin >> std::hex >> temp;
        if( temp == end_ ) {
            break;
        }   
        if( temp >= word_lower_limit && temp < word_upper_limit )
            memory[ ins_cnt++ ] = temp;
        else 
            continue;
    }

    ins_cnt = 0;
    std::cout << std::setfill( ' ' );
    std::cout <<  std::setw( 5 ) << std::left << "***" 
        << "Program loaded into memory" 
        << std::setw( 5 ) << std::right << "***\n";
    
    std::cout <<  std::setw( 5 ) << std::left << "***" 
        << "Program execution starts..." 
        << std::setw( 5 ) << std::right << "***\n";

    execute();
    std::cout << std::endl;
}

void SML::execute() 
{
    int divisor; 
    size_word &ins_cnt = registers[ static_cast<unsigned>( INSTRUCTION_COUNTER ) ];
    size_word &ins_reg = registers[ static_cast<unsigned>( INSTRUCTION_REGISTER ) ];
    
    while( memory[ ins_cnt ] != 0  ) 
    {
        ins_reg = memory[ ins_cnt++ ];

        if( ins_reg < 1000 ) divisor = 0x10; 
        else if( ins_reg >= 1000 && ins_reg < 10000 ) divisor =  0x100; 
        else if( ins_reg >= 10000 && ins_reg < 100000 ) divisor =  0x1000; 

        Evaluator eval( *this ); // create an instance of evaluator
        try
        {
            if( eval.evaluate( *this, ins_reg, divisor ) == 0 )
                break;
        } 
        catch ( std::invalid_argument &e )
        {
            std::cout << e.what() << "\n";
        }
        if( debug )
            memory_dump();
    }
}

void SML::memory_dump() const
{
    std::cout << "\nREGISTERS:\n";

    std::cout << std::setw( 25 ) << std::left << std::setfill( ' ' ) << "accumulator" << std::showpos
        << std::setw( 5 ) << std::setfill( '0' ) << std::internal << registers[ 0 ] << '\n';

    std::cout << std::setw( 28 ) << std::left << std::setfill( ' ' ) 
    << "instruction counter" << std::noshowpos <<  std::setfill( '0' ) 
    << std::right << std::setw( 2 ) << registers[ 1 ] << '\n';

    std::cout << std::setw( 25 ) << std::left << std::setfill( ' ' ) 
    << "instruction register" << std::showpos << std::setw( 5 ) << std::setfill( '0' ) 
    << std::internal << registers[ 3 ] << '\n';

    std::cout << std::setw( 28 ) << std::left << std::setfill( ' ' ) 
    << "operation code" << std::noshowpos <<  std::setfill( '0' ) 
     << std::right << std::setw( 2 ) << registers[ 4 ] << '\n';

    std::cout << std::setw( 28 ) << std::left << std::setfill( ' ' ) 
    << "operand" << std::noshowpos <<  std::setfill( '0' ) 
     << std::right << std::setw( 2 ) << registers[ 5 ] << '\n';

    std::cout << "\n\nMEMORY:\n";
    std::cout << "  ";

    for( int i = 0; i != 10; ++i ) 
        std::cout << std::setw( 6 ) << std::setfill( ' ') << std::right << i;
    for( size_word i = 0; i != memory_size; ++i ) 
    {
        if( i % 10 == 0 )
            std::cout << "\n" << std::setw( 3 ) << std::setfill( ' ' ) << i << " ";
        std::cout << std::setw( 5 ) << std::setfill( '0' ) << std::showpos << std::internal << memory[ i ] << " ";
    }
    std::cout << std::endl;
}

SML::~SML()
{
    // resets all the registers
    set_registers();

    // free the memory
    delete [] memory;
}

Evaluator.h

#ifndef SML_EVALUATOR_H_
#define SML_EVALUATOR_H_

#include <iostream>
#include <stdint.h>

typedef int32_t size_word; 
constexpr size_word instruction_max_sixe = 70;

class SML;

class Evaluator
{
    public:
        Evaluator() = default;
        Evaluator( const SML & );
        int evaluate( SML &s, const int ins_reg, const int divisor );
    
    private:
        void read( SML &s, const int opr );
        void write( SML &s, const int opr );
        void read_str( SML &s, const int opr );
        void write_str( SML &s, const int opr );
        void load( SML &s, const int opr );
        void store( SML &s, const int opr );
        void add( SML &s, const int opr );
        void subtract( SML &s, const int opr );
        void multiply( SML &s, const int opr );
        void divide( SML &s, const int opr );
        void modulo( SML &s, const int opr );
        void branch( SML &s, const int opr );
        void branchneg( SML &s, const int opr );
        void branchzero( SML &s, const int opr );
        void newline( SML &s, const int opr );
        void smldebug( SML &s, const int opr );
        bool division_by_zero( SML &s, const int opr );

        void (Evaluator::*instruction_set[ instruction_max_sixe ])( SML &, int );
};

#endif

Evaluator.cpp

#include "evaluator.h"

#include "sml.h"

Evaluator::Evaluator( const SML &s )
{
    instruction_set[ s.read_ ] = &Evaluator::read;
    instruction_set[ s.write_ ] = &Evaluator::write;
    instruction_set[ s.read_str_ ] = &Evaluator::read_str;
    instruction_set[ s.write_str_ ] = &Evaluator::write_str;
    instruction_set[ s.load_ ] = &Evaluator::load;
    instruction_set[ s.store_ ] = &Evaluator::store;
    instruction_set[ s.add_ ] = &Evaluator::add;
    instruction_set[ s.subtract_ ] = &Evaluator::subtract;
    instruction_set[ s.multiply_ ] = &Evaluator::multiply;
    instruction_set[ s.divide_ ] = &Evaluator::divide;
    instruction_set[ s.modulo_ ] = &Evaluator::modulo;
    instruction_set[ s.branch_ ] = &Evaluator::branch;
    instruction_set[ s.branchneg_ ] = &Evaluator::branchneg;
    instruction_set[ s.branchzero_ ] = &Evaluator::branchzero;
    instruction_set[ s.newline_ ] = &Evaluator::newline;
    instruction_set[ s.sml_debug_ ] = &Evaluator::smldebug;
}

int Evaluator::evaluate( SML &s, const int ins_reg, const int divisor) 
{
    size_word &opr_code = s.registers[ static_cast<unsigned>( OPERATION_CODE ) ];
    size_word &opr = s.registers[ static_cast<unsigned>( OPERAND ) ];
    opr_code = ins_reg /  divisor;
    opr = ins_reg %  divisor;

    if( opr_code == s.halt_ )
        return 0;
    else 
        (this->*(instruction_set[ opr_code ]))( s, opr );
        return 1;
}

void Evaluator::read( SML &s, const int opr ) 
{  
    std::cin >> s.memory[ opr ];
}

void Evaluator::write( SML &s, const int opr ) 
{ 
    std::cout << s.memory[ opr ]; 
}

void Evaluator::read_str( SML &s, const int opr )
{
    std::cin >> s.temp_str;
    s.memory[ opr ] = s.temp_str.size();

    for( std::string::size_type i = 1; i != s.temp_str.size() + 1; ++i )
        s.memory[ opr + i ] = int( s.temp_str[ i - 1 ] );
}

void Evaluator::write_str( SML &s, const int opr )
{
    for( int i = 0; i != s.memory[ opr ] + 1; ++i )
        std::cout << char( s.memory[ opr + i ]);
}

void Evaluator::load( SML &s, const int opr )
{
    size_word &accumulator = s.registers[ static_cast<unsigned>( ACCUMULATOR ) ];
    accumulator = s.memory[ opr ];
}

void Evaluator::store( SML &s, const int opr )
{
    size_word &accumulator = s.registers[ static_cast<unsigned>( ACCUMULATOR ) ];
    s.memory[ opr ] = accumulator;
}

void Evaluator::add( SML &s, const int opr )
{
    size_word &accumulator = s.registers[ static_cast<unsigned>( ACCUMULATOR ) ];
    accumulator += s.memory[ opr ];
}

void Evaluator::subtract( SML &s, const int opr )
{
    size_word &accumulator = s.registers[ static_cast<unsigned>( ACCUMULATOR ) ];
    accumulator -= s.memory[ opr ];
}

void Evaluator::multiply( SML &s, const int opr )
{
    size_word &accumulator = s.registers[ static_cast<unsigned>( ACCUMULATOR ) ];
    accumulator *= s.memory[ opr ];
}

void Evaluator::divide( SML &s, const int opr )
{
    if( division_by_zero( s, opr ) )
        throw std::invalid_argument( "Division by zero: Program terminated abnormally." );

    size_word &accumulator = s.registers[ static_cast<unsigned>( ACCUMULATOR ) ];
    accumulator /= s.memory[ opr ];
}

void Evaluator::modulo( SML &s, const int opr )
{
    if( division_by_zero( s, opr ) )
        throw std::invalid_argument( "Division by zero: Program terminated abnormally." );

    size_word &accumulator = s.registers[ static_cast<unsigned>( ACCUMULATOR ) ];
    accumulator /= s.memory[ opr ];
}

bool Evaluator::division_by_zero( SML &s, const int opr )
{
    return ( s.memory[ opr ] == 0 );
}

void Evaluator::branchneg( SML &s, const int opr )
{
    size_word &accumulator = s.registers[ static_cast<unsigned>( ACCUMULATOR ) ];
    if( accumulator < 0 ) 
        branch( s, opr );
}

void Evaluator::branchzero( SML &s, const int opr )
{
    size_word &accumulator = s.registers[ static_cast<unsigned>( ACCUMULATOR ) ];
    if( accumulator == 0 ) 
        branch( s, opr );
}

void Evaluator::branch( SML &s, const int opr )
{
    size_word &ins_cnt = s.registers[ static_cast<unsigned>( INSTRUCTION_COUNTER ) ];
    s.registers[ static_cast<unsigned>( TEMPORARY_COUNTER ) ] = ins_cnt;
    ins_cnt = opr;
    s.execute();
    ins_cnt =  s.registers[ static_cast<unsigned>( TEMPORARY_COUNTER ) ];  
}

void Evaluator::newline( SML &s, const int opr )
{
    std::cout << '\n' << std::flush;
}

void Evaluator::smldebug( SML &s, const int opr )
{
    if ( opr == 1 ) s.debug = true;
    else if ( opr == 0 ) s.debug = false;
}

main.cpp

#include "sml.h"

int main()
{
    SML sml(1000, -999999, 999999 );
    sml.display_welcome_message();
    sml.load_program();
}

Below are instructions written to test the machine

Tests

0xA60 // read a value and store in address 60( written to index 96(decimal) in the array, 
0xA61 // read another value and store in address 61
0x1460 // write the value stored in address 60 to the accumulator
0x1e61 // add the value stored in address 61 to the accumulator
0x320 // print a newline
0x1562 // store the value in the accumulatore to address 62
0xb62 // write the value in address 62 to the screen
0x320 // print a newline
0xc67 // read a string and store it size at address 67, the characters would be stored from 68 to end of character
0xd67 // write the characters to screen
0x2c1 // turn on debug
-0x1869f // start execution
\$\endgroup\$
12
  • 1
    \$\begingroup\$ Can you give an example set of instructions for testing? \$\endgroup\$ – Parekh Nov 12 '20 at 4:17
  • 1
    \$\begingroup\$ @Aryan Parekh Should I add it to the question or comment it here? \$\endgroup\$ – theProgrammer Nov 12 '20 at 4:33
  • 1
    \$\begingroup\$ Anywhere you'd like, Adding it to the question would be better I think \$\endgroup\$ – Parekh Nov 12 '20 at 4:34
  • 3
    \$\begingroup\$ @AryanParekh. I editted the question. \$\endgroup\$ – theProgrammer Nov 12 '20 at 5:53
  • 2
    \$\begingroup\$ The program crashes, outputs something else. When I tried to debug your program, I see that you have an unhandled exception thrown in your evaluate() function. 0xC0000005 \$\endgroup\$ – Parekh Nov 12 '20 at 7:23
11
\$\begingroup\$

Just a few things

Use Raw string literals

std::cout << "***" << " WELCOME TO SIMPLETRON! " << "***\n\n";
    std::cout << std::setw(5) << std::left << "***"
        << "Please enter your program one instruction"
        << std::setw(5) << std::right << "***\n";

    std::cout << std::setw(5) << std::left << "***"
        << "(or data word) at a time. I will type the"
        << std::setw(5) << std::right << "***\n";

    std::cout << std::setw(5) << std::left << "***"
        << "location number and a question mark (?)."
        << std::setw(6) << std::right << "***\n";

    std::cout << std::setw(5) << std::left << "***"
        << "You then type the word for that location"
        << std::setw(6) << std::right << "***\n";

    std::cout << std::setw(5) << std::left << "***"
        << "Type the sentinel -0x1869F to stop entering"
        << std::setw(5) << std::right << "***\n";

    std::cout << std::setw(5) << std::left << "***"
        << "your program"
        << std::setw(5) << std::right << "***";

    std::cout << "\n\n" << std::flush;

This can get extremely difficult to maintain. You can simply use string literals to make your life easy

const char* welcome_msg = R"""(

***         WELCOME TO SIMPLETRON!               ***

***  Please enter your program one instruction   ***
***  (or data word) at a time. I will type the   ***
***  location number and a question mark (?).    ***
***  You then type the word for that location    ***
***  Type the sentinel -0x1869F to stop entering ***
***  your program                                ***

)"""
std::cout << welcome_msg;

Simplify

    registers[static_cast<unsigned>(ACCUMULATOR)] = 0;
    registers[static_cast<unsigned>(INSTRUCTION_COUNTER)] = 0;
    registers[static_cast<unsigned>(TEMPORARY_COUNTER)] = 0;
    registers[static_cast<unsigned>(INSTRUCTION_REGISTER)] = 0;
    registers[static_cast<unsigned>(OPERATION_CODE)] = 0;
    registers[static_cast<unsigned>(OPERAND)] = 0;

Instead of casting it to unsigned every time you use something from the enum, why not declare it unsigned first?

enum REGISTERS : unsigned
{
    ACCUMULATOR = 0,
    INSTRUCTION_COUNTER = 1,
    TEMPORARY_COUNTER = 2,
    INSTRUCTION_REGISTER = 3,
    OPERATION_CODE = 4,
    OPERAND = 5
};

Also, you don't have to specify the values here since they are continuous. That means this is the same as

enum REGISTERS : unsigned
{
    ACCUMULATOR,
    INSTRUCTION_COUNTER ,
    TEMPORARY_COUNTER,
    INSTRUCTION_REGISTER,
    OPERATION_CODE,
    OPERAND
};

Use a loop

    registers[ACCUMULATOR] = 0;
    registers[INSTRUCTION_COUNTER] = 0;
    registers[TEMPORARY_COUNTER] = 0;
    registers[INSTRUCTION_REGISTER] = 0;
    registers[OPERATION_CODE] = 0;
    registers[OPERAND] = 0;

Take advantage of the fact that these are all numbered from 1 to 5.

    for (int i = ACCUMULATOR; i <= OPERAND; i++)
        registers[i] = 0;

Comparing size_t and int32_t

int32_t has a fixed width of 32.
size_t is either 32 / 64 bits, depending on the platform.
Comparing both of them freely can sometimes be dangerous.

s.memory[opr] = s.temp_str.size();
 in32_t       =    size_t

If size_t (although highly unlikely, possible ) exceeds the max size of int32_t, overflow! What I like to do is to keep a custom macro like _DEBUG_, and then use #ifdef to check for this.

#ifdef _DEBUG_
if ( s.temp_str.size() > INT32_MAX ) // handle it here

#endif // _DEBUG_
\$\endgroup\$
8
  • 4
    \$\begingroup\$ You may have swapped one bug for another with the size_t debug code at the end of this answer - unless you can somehow prove that SIZE_MAX >= INT32_MAX. 16-bit machines may be rare in your world, but they do still exist... \$\endgroup\$ – Toby Speight Nov 12 '20 at 8:10
  • 3
    \$\begingroup\$ If int32 is wider than size@t then s.temp_str.size() > INT32_MAX can never be true. Actually, that's not really a problem is it? Just a compiler warning, that can be suppressed. \$\endgroup\$ – Toby Speight Nov 12 '20 at 10:14
  • 2
    \$\begingroup\$ Instead of writing #ifdef _DEBUG_ if (fault_condition) ... #endif, write assert(!fault_condition). It is meant exactly for this kind of debug build testing. \$\endgroup\$ – G. Sliepen Nov 12 '20 at 12:34
  • 3
    \$\begingroup\$ @AryanParekh You are right, but I would avoid making the behaviour of the program change depending on whether it is a debug or release build. And if you are debugging the program, having it abort due to an assertion failure is a nice way of getting a coredump and allowing actual debugging to take place. \$\endgroup\$ – G. Sliepen Nov 12 '20 at 13:10
  • 2
    \$\begingroup\$ On that enum, I'd add a value called "LAST_VALUE" at the end, and then in the loop, just loop on i < LAST_VALUE (note not <=). That way if you add more enum values you don't have to find every place where you loop through them and change them all. \$\endgroup\$ – Darrel Hoffman Nov 12 '20 at 21:02
8
\$\begingroup\$

Overall Observations

I do see some serious improvement here over the first question. Did you find it any easier to write this second version?

The program isn't exactly user friendly, when it is executing the SML program it doesn't prompt the user for input on read statements.

You're working on your object oriented programming in C++ and this is a good thing!

There seems to be rather strong dependencies between the 2 classes this is known as tight coupling and generally indicates there is a problem with the design of the objects. I haven't used friend in at least 27 years except for defining the << operator in classes that need specialized output. The responsibilities of the classes need to be segmented better.

I this point I think it would be helpful if you learned the 5 SOLID programming principles. SOLID is a mnemonic acronym for five design principles intended to make software designs more understandable, flexible and maintainable. This will help you design your objects and classes better.

  1. The Single Responsibility Principle - A class should only have a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class. This particular principle can be applied to functional programming as well.
  2. The Open–closed Principle - states software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification.
  3. The Liskov Substitution Principle - Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program.
  4. The Interface segregation principle - states that no client should be forced to depend on methods it does not use.
  5. The Dependency Inversion Principle - is a specific form of decoupling software modules. When following this principle, the conventional dependency relationships established from high-level, policy-setting modules to low-level, dependency modules are reversed, thus rendering high-level modules independent of the low-level module implementation details.

Possible there could be a third class that represents the processor. You could also create an enum that is shared by both SML and Evaluator for the indexes into instruction_set.

Complexity of void SML::memory_dump() const

Looking at void SML::memory_dump() const I actually see 2 separate functions if the Single Responsibility Principle is applied

  1. dump_registers()
  2. dump_memory()

The outer function that contains both function could be dump_current_program_state().

void SML::dump_current_program_state() const
{
    dump_registers();
    memory_dump();
}

void SML::dump_registers() const
{
    std::cout << "\nREGISTERS:\n";

    std::cout << std::setw(25) << std::left << std::setfill(' ') << "accumulator" << std::showpos
        << std::setw(5) << std::setfill('0') << std::internal << registers[0] << '\n';

    std::cout << std::setw(28) << std::left << std::setfill(' ')
        << "instruction counter" << std::noshowpos << std::setfill('0')
        << std::right << std::setw(2) << registers[1] << '\n';

    std::cout << std::setw(25) << std::left << std::setfill(' ')
        << "instruction register" << std::showpos << std::setw(5) << std::setfill('0')
        << std::internal << registers[3] << '\n';

    std::cout << std::setw(28) << std::left << std::setfill(' ')
        << "operation code" << std::noshowpos << std::setfill('0')
        << std::right << std::setw(2) << registers[4] << '\n';

    std::cout << std::setw(28) << std::left << std::setfill(' ')
        << "operand" << std::noshowpos << std::setfill('0')
        << std::right << std::setw(2) << registers[5] << '\n';
}

void SML::memory_dump() const
{
    std::cout << "\n\nMEMORY:\n";
    std::cout << "  ";

    for (int i = 0; i != 10; ++i)
        std::cout << std::setw(6) << std::setfill(' ') << std::right << i;
    for (size_word i = 0; i != memory_size; ++i)
    {
        if (i % 10 == 0)
            std::cout << "\n" << std::setw(3) << std::setfill(' ') << i << " ";
        std::cout << std::setw(5) << std::setfill('0') << std::showpos << std::internal << memory[i] << " ";
    }
    std::cout << std::endl;
}

Magic Numbers

You have done a good job of preventing magic numbers in sml.h, however, there are Magic Numbers in the main() function (1000, -999999, 999999) as well as in SML::memory_dump() (25, 5, 28, 10), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

In main() you can create constexpr memory_size = 1000; for the first value, I'm not sure what the -999999 and 9999999 values should be called.

Registers Uninitialized

In the following constructor I don't see where the registers get initialized:

SML::SML(SML&& s)
{
    swap(*this, s);
    memory = new size_word[memory_size];
    std::move(s.memory, s.memory + s.memory_size, memory);
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ In the following constructor I don't see where the registers get initialized: check swap method, I initialized it there. \$\endgroup\$ – theProgrammer Nov 12 '20 at 16:35
  • 2
    \$\begingroup\$ Are you suggesting that a read instruction should print a prompt on its own? The simulated machine has a write instruction. If a program wants to prompt the user, it should do so itself, using the available instructions, just like asm for mainstream OSes. For similar reasons why ISO C scanf just reads. (Although to be fair, this is clearly a toy machine, and over-simplified syscall interfaces are the norm, like in MARS, the MIPS simulator: courses.missouristate.edu/KenVollmar/Mars/Help/SyscallHelp.html - no facilities for controlling the terminal (no echo), or cursor movement). \$\endgroup\$ – Peter Cordes Nov 12 '20 at 20:09
  • 1
    \$\begingroup\$ @PeterCordes I was suggesting that. The first time I ran the program with the example input it took me a while to figure out why I wasn't getting any output after the the SML started processing. \$\endgroup\$ – pacmaninbw Nov 12 '20 at 21:23
  • 1
    \$\begingroup\$ @pacmaninbw: Ah, I hadn't looked at the example code from the question. That's the fault of a non-user-friendly example, not of the ISA / system-call interface design. (Unless the intended use-case is even more toy-like, with read forcing printing a prompt, so you don't get to choose to e.g. input 3 numbers without separate prompts. That would be a valid option; not the one MARS made, but worth considering.) \$\endgroup\$ – Peter Cordes Nov 15 '20 at 21:57
8
\$\begingroup\$

Use enum class instead of enum

It's a good habit to make the enum a enum class. I can't tell you how many times I've needed to untangle two or more state-machines that used similarly or identically named states that conflicted in value. This will prevent you from passing unchecked values as registers.

Use standard containers

Your memory variable could be a std::vector - reserve the size during the ctor, and then when the sml object is destroyed, it's automatically cleaned up.

Likewise, you could use std::array or one of the maps for the registers. std::array can be made constexpr, so if you compile with c++2a/c++20, you could potentially verify your entire program at compilation instead of at run time.

Both of these should make the copy and move operators a bit easier to tame. As well

Use Standard Algorithms

Particularly in the Evaluator you could get practiced with standard algorithms. This isn't necessarily a speedup, but it's good practice.

void Evaluator::write_str( SML &s, const int opr )
{
    for( int i = 0; i != s.memory[ opr ] + 1; ++i )
        std::cout << char( s.memory[ opr + i ]);
}

void Evaluator::write_str(SML &s, const Operand o){
    auto out_itr = std::ostream_iterator<char>(std::cout, "");
    std::copy(s.memory.cbegin(), std::next(s.memory.cbegin() to_underlying(O)), out_itr);
}

An extra benefit of continued use of the algorithms is consistency, and conveying intent. If you're finding something, use find or find_if if you doing something for each item, like printing out, you could use for_each. You can also rebuild the standard algorithms, they're pretty entry level template functions, that are pretty easy to dip your toes.

Defined elsewhere - to convert enum class to int

#include <type_traits>

template <typename E>
constexpr auto to_underlying(E e) noexcept
{
    return static_cast<std::underlying_type_t<E>>(e);
}

Add a std::ostream member, and pipe through that instead of std::cout

This is a little nicety which goes a long way. By adding std::ostream member to your classes, and then constructing to default to std::cout, you can then output to whatever you want! Got a file you want to pipe to? Great. How about a stream that can be unit tested? Sure. Once you do this, then you can add auto-building and testing, saving you time from needing to manually check if that little change you made actually broke everything.

Use unique_ptr

Bonus edit: since I remembered about this - if you don't want to use the standard containers, then you should really manage your data (registers and memory) with unique_ptrs. new and delete are often treated as code smells, and with good reason. It's really easy to try to double-free or forget to delete and leak memory, both of these are very bad.

\$\endgroup\$
9
  • 2
    \$\begingroup\$ You could std::cout.write(). \$\endgroup\$ – Deduplicator Nov 12 '20 at 17:29
  • 1
    \$\begingroup\$ Great review. I would add that, I want to make minimal use of the Standard algorithm as possible. \$\endgroup\$ – theProgrammer Nov 12 '20 at 17:31
  • 1
    \$\begingroup\$ Why would you want a hash table or tree for the register file? How is that better than a std::array or std::vector with enum indices? The current design (with static_cast<unsigned> on every enum) is bad, but there's no need to over-complicate it with fancy data structures when you know register-numbers have a small contiguous range starting at 0. Just simple containers like vector or array for registers should be good, like you wisely suggests for memory. \$\endgroup\$ – Peter Cordes Nov 12 '20 at 20:13
  • 3
    \$\begingroup\$ @theProgrammer: @Aryan's answer already showed how to simplify the enum to avoid casting every time you use it. The values are signed-positive anyway so casting seems pointless. The overall design isn't actually bad, just the detail of cluttering up your source code with casts. An array is actually good for the register file IMO, although a standard container like std::array could be useful as this answer points out (along with the bad IMO suggestion of std::unordered_map). \$\endgroup\$ – Peter Cordes Nov 12 '20 at 21:08
  • 2
    \$\begingroup\$ @PeterCordes std::array is probably the ideal container. \$\endgroup\$ – Alex Shirley Nov 12 '20 at 22:16

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.