Implement class for assembling instructions into bit strings

[goldsborough]

source/arch/riscv/instruction-node.cpp:

1. Please use more descriptive variable names. Variables are like children and you would not call your child i. This refers to:
   • s/immToI/immediateToIFormat
   • s/vec/vector
2. Use std::size_t instead of int for for-loop variables here.
3. About your str2int: I'm not sure you know what you're doing here, but your str2int is the djb2 string hash function. There are multiple things wrong about this:
   • I see zero comments about why/what you are doing. I just happen to know this hash function and understand that you want to hash the strings to switch-case, but I think this is not common knowledge, so commenting is quite necessary here.
   • If you want to hash strings, you should use std::hash<std::string>{}(string).
   • Of course, you could also just do if-else and ==.
   • Since performance is not relevant here, the best solution would be:
     1. Use if-else and refactor the whole thing into a new function (which you should have done anyway).
     2. Store a hashmap from names to functions and let it do the hashing yourself.
   • Please realize that anytime you are using the C api of things in C++ (c_str()) you are probably doing something wrong! You should have stopped at that point.
4. Why are there no comments anywhere? I'm not supposed to have to sit 5 minutes in front of your code and try to understand what it's doing.
5. The use of auto is questionable. For example, you write InstructionKey instructionKey = _instructionInformation.getKey(). Here, I know the result will be an instructionKey, so it's ok to use auto. But then you write auto boolResult = assembler(instructionKey, args);. Here, I have absolutely no idea what assembler returns and boolResult only hints at it, so I have to waste time looking up the interface, whereas std::vector<bool> result = ... would make more sense, since I see that it is a std::vector<bool>, and the result :blush:
6. There is no need to use at if you don't explicitly want bounds checking. You should use [], and assertions if you want to check bounds.
7. Do not name your children k.
8. Also, a note on refactoring. A function is supposed to do one thing and one thing only

era-gp-sim/source/arch/riscv/formats.cpp:

• s/Utility::converToBin/Utility::convertToBinary ... please ...
• I don't think pushBackFromEnd should be in Utility. There's nothing "general" about it that could be reused for many types, under many circumstances.
• I think the for loops are too long to be on a single line.
• I'm not saying the code is hard to understand. I'm just saying that if anyone, including you, were to come back to this code in, say, two months, you would have zero idea about what it does.
• The functions are too long, please refactor them.
• You are taking args as const copy. I suppose you forgot to take them by reference &.
• Use [] instead of at()
• Please comment more.
• The documentation in the header file is insufficient. Please have a look how everything else in the project is documented.
• The destructor in Format is not virtual, so your code is leaking every single byte you allocate :blush:

era-gp-sim/include/arch/common/register-node.hpp:

• The functions you wrote are not documented (with doxygen). I should theoretically not even have to review them.
• Why are they in the header file?
• s/sz/size
• Show me the comments, show me the comments
• Please use full words for your variable names.
• Please do not use C functions like std::stoi when writing C++. Use streams.
• Sometimes you use int, sometimes std::string::size_type. Just use std::size_t.
• I have no idea what your try catch thing does, it just looks wrong.
• Why are there no braces around your if and else clauses?
• I have no idea what the code is doing because it is not documented.

era-gp-sim/source/common/utility.hpp: There are quite a few things wrong with convertToBin:

• You take the binary as a reference (output parameter). Why not ... return a new std::vector<bool>??
• Use typename = std::enable_if_t<std::is_integral<T>::value> to ensure the input template parameter is actually integral.
• You must be kidding about const T& value and then T copyValue = value? Why not just copy the value in the parameter list? void convertToBin(std::vector<bool>& binary, T value...
• No comments and no documentation = no problems?

The code has very many issues and is not ready. Main things to look into:

1. Document your functions properly, like everyone else.
2. Use comments to make it clear to other human beings what you are trying to achieve.
3. Try to keep functions under 20-30 lines. If you are writing more than that many lines in your function, you're probably doing more than one thing and you could refactor it into a new function.
4. Your variable names should be or consist of words that can be found in the Oxford English Dictionary. Otherwise I will write a script that downloads the Oxford English Dictionary and tests your code against it.
5. Do not, ever, in the world, while you are alive, while I'm alive, use C functions in C++.

Thanks and good luck. Take this potato on your journey: 🍠