6
\$\begingroup\$

I'm a newb. I learned some C in high school, and using that to learn C++. I really didn't understand the use of OO, but I had a major enlightenment while working on this simple piece of code. I have refactored it in a more OO style and simplified it using C++ idioms to where it looks as clean as I can make it. I'm hoping for advice in terms of understanding basic C++/OO idioms that would be applicable to other projects. I'm not looking for advanced algorithm optimization. Also, I know well how to use sed and tr. :) This was a learning exercise.

This project operates on a struct containing a list of assembler commands and associated opcodes. The opcodes have shorthand characters, G through O which mark registers. The function of the program is to expand the list by replacing each of those shorthands with an expanded list of opcodes containing each valid register code. I replaced address codes with 0xAAAA as I didn't want to expand the entire address space many times over! The purpose was to build a fuzzer for a decompiler to ensure it processed all opcodes correctly.

Initially, I wrote this more C style, passing a lot of references to functions for return values. I suddenly understood that by moving my functions and data into a class, I could simplify the code by not having to pass a bunch of extra references. So I built a class to contain a queue. Load an element into the queue, and read the data out. Reading the data out calls a method that performs the work by iterating over the internal queue buffer and pushing processed data back into queue until it's complete. I have a feeling recursion could also be used here, but it's not obvious to me how to make that work. I've tried to ensure const correctness, pass by reference to avoid copies where possible, and write in such a way as to get return value optimization. One thing I see now I could improve on is that I duplicated the push/pop mechanism of a std::vector. I probably could have used inheritance to implement that, but I don't quite understand how to do that yet.

#include <iostream>
#include <vector>

const Instruction instructions[] = {    // define each opcode instruction name pair.
 { "ANDC #xx:8,EXR", "014106MM"}
,{ "BAND #xx:3,@ERd", "7CG076P0"}
,{ "BAND #xx:3,@aa:8", "7ENN76P0"}
,{ "BAND #xx:3,@aa:16", "6A10NNNN76P0"}
,{ "BAND #xx:3,@aa:32", "6A30NNNNNNNN76P0"}
,{ "BCLR #xx:3,@aa:16", "6A18NNNN72P0"}
};

struct Instruction {
    std::string name;     //instruction name, longest I saw was 27 characters long.
    std::string bytecode; //bytecode
};

class InstructionProcessQueue {
public:

    void push(Instruction elementToAdd) {
        processQueue.push_back(elementToAdd);
    }

    Instruction pop() {
        if (empty()) exit(__LINE__); // Error check

        currentInstruction = processQueue.back(); // Pull an element from the queue
        processQueue.pop_back();

        for (char& bytecodeElement : currentInstruction.bytecode) {
            switch (bytecodeElement) {
            case 'O': //ERn: 3 bits w preceding 1
            case 'Q':
                expandOpcode(bytecodeElement, 8, 15);
                currentInstruction = processQueue.back();
                processQueue.pop_back();
                break;
            case 'G':  //ERn: 3 bits with preceding 0
            case 'P':  //3 bits with preceding 0
                expandOpcode(bytecodeElement, 0, 7);
                currentInstruction = processQueue.back();
                processQueue.pop_back();
                break;
            case 'H': //Rn: 4 bits
                expandOpcode(bytecodeElement, 0, 15);
                currentInstruction = processQueue.back();
                processQueue.pop_back();
                break;
            case 'L': //Address space, 8, 16, 32 bits. 
            case 'M':
            case 'N':
                expandOpcode(bytecodeElement, 10, 10); // Just insert 0xA
                currentInstruction = processQueue.back();
                processQueue.pop_back();
                break;
            }
        }
        return currentInstruction;
    }

    const bool empty() const {
        return processQueue.empty();
    }

private:
    const std::string Hex = "0123456789ABCDEF";
    std::vector<Instruction> processQueue;
    Instruction currentInstruction;

    void expandOpcode(char& bytecodeElement, const int& begin, const int& end) {
        for (int i = begin; i <= end; i++) {
            bytecodeElement = Hex[i];
            processQueue.push_back(currentInstruction);
        }
    }
};

int main() {
    std::vector<Instruction> instrOutput;
    InstructionProcessQueue queue;

    for (const auto& i : instructions) {    //Iterate over the const instruction list
        queue.push(i);                      //Put the current element in the process queue

        while (!queue.empty())              //Process the queue until empty
            instrOutput.push_back(queue.pop());

        for (auto& i : instrOutput) {       //Iterate the output and clear it for the next loop
            std::cout << i.name << ' ' << i.bytecode << std::endl;
        }
        instrOutput.clear();
    }
}
\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

For a beginner in C++, your code is looking very good!

Define types before you use them

The code you posted doesn't compile because you are using the type Instruction before it is defined.

Consider throwing exceptions on errors

You call exit(__LINE__) if someone tries to pop from an empty queue, but that has several problems. First, you should #include <cstdlib> and call std::exit() instead, as otherwise the C++ standard does not guarantee that this function exists.

Second, return EXIT_FAILURE instead of __LINE__. Exit codes only have a limited range of possible values, but __LINE__ could be anything. If it has the wrong value it could potentially be truncated to zero, which would indicate success instead of failure.

Third, consider throwing an exception instead. If it is not handled, it will cause the program to terminate as well, potentially with a better error message if you use a standard exception type, like std::logic_error. It also gives the caller the opportunity to catch the exception, and potentially recover from the error.

Add a default case to your switch statement

You might want to add a default: break; to your switch statement to make it explicit that you want to ignore characters that don't match any case statement.

Note that some compilers might also warn if they see that you don't handle all possible values.

Don't store data unnecessarily

The variable currentInstruction is only used in pop() and in expandOpcode(), and the latter is only ever called from pop(). So outside of those functions, there is no need to store a currentInstruction. Make currentInstruction a local variable inside pop(), and also pass it as a parameter to expandOpcode().

Hex is only needed inside expandOpcode(), so move it into that function. Also make it static.

In main(), the vector instrOutput is not necessary. Instead you can just print the result from pulling from queue directly:

InstructionProcessQueue queue;

for (const auto& instr_in : instructions) {
    queue.push(instr_in);

    while (!queue.empty()) {
         auto instr_out = queue.pop();
         std::cout << instr_out.name << ' ' << instr_out.bytecode << '\n';
    }
}

Add commas after array items

When you define an array, it is more common to add the comma after each item. You are also allowed to add a comma after the last element. So:

const Instruction instructions[] = {
    { "ANDC #xx:8,EXR", "014106MM"},
    { "BAND #xx:3,@ERd", "7CG076P0"},
    …
    { "BCLR #xx:3,@aa:16", "6A18NNNN72P0"},
};

Avoid unnecessary copies

Your code works fine, but in several places it is making copies of Instructions unnecessarily. Since Instruction is a "large" object, those copies can have a performance impact. You can avoid that and make your program more efficient. First, in push(), pass elementToAdd() as a const reference:

void push(const Instruction& elementToAdd) {
    …
}

This avoids the caller having to make a copy, it will just pass a reference. Inside push(), it will still make a copy when adding the element to processQueue, but that is fine.

Each time you pop an item from the queue you are making a copy as well. Here we can use std::move(), like so:

currentInstruction = std::move(processQueue.back());
processQueue.pop_back();

Also, since you repeat this pattern 5 times, you might want to put this into a private member function.

Don't pass small values by reference

While copying large objects can be inefficient, and thus passing them by reference is generally better, you should not pass everything by reference. Small things like int and float are better passed by value. So in expandOpcode(), pass begin and end by value.

Of course, bytecodeElement should still be passed by reference because you want to mutate the value in the caller.

Inheriting from std::vector

One thing I see now I could improve on is that I duplicated the push/pop mechanism of a std::vector. I probably could have used inheritance to implement that, but I don't quite understand how to do that yet.

It's probably better not to inherit from std::vector. This might expose all kinds of undesired functionality. So your current approach of having a private member variable is correct.

Consider using std::queue

Since you are using processQueue stricly as a queue, use std::queue instead of std::vector.

Expand on push or on pull?

Your example code always pushes one Instruction and then pulls from the queue until it is empty. But what if you would first push all elements of instructions[], and only then start pulling from the queue? The output would be different. I'm not sure that is what you want. Alternatively, you could expand the instructions inside push() instead of doing it inside pull().

If the idea is to only expand instructions one at a time, and not have the queue fill up with the expansion of multiple instructions, consider whether you need a class at all, and maybe you just should write a free function expand(), that basically combines your push() and the pop() loop into one:

static void expandOpcode(char& bytecodeElement, int begin, int 
end, const Instruction& instruction, std::vector<Instruction>& result) {
    static const std::string Hex = "…";

    for (int i = begin; i <= end; i++) {
        bytecodeElement = Hex[i];
        result.push_back(instruction);
    }
}

std::vector<Instruction> expand(Instruction instruction) {
    std::vector<Instruction> result;

    for (char& bytecodeElement : instruction.bytecode) {
        switch (bytecodeElement) {
        case 'O':
        case 'Q':
            expandOpcode(bytecodeElement, 8, 15, instruction, result);
            break;
        …
        }
    }

    return result;
}

And then your main() could look like:

int main() {
    for (const auto& instr_in : instructions) {
        for (const auto& instr_out : expand(instr_in)) {
            std::cout << instr_out.name << ' ' << instr_out.bytecode << '\n';
        }
    }
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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