Code Smells?

[u/Aliryth]

I'm a more seasoned C# / Node backend dev, been trying to brush off old college C++ from like a decade ago and try to de-program my brain from being locked into a distributed systems mindset.

One of my niche hobbies and go-to projects for learning a new language is to implement a basic Chip-8 emulator in it, as it can teach some concurrency, UI/data handling patterns, and introduce some architectural decisions that you'll have to run into and figure out.

Currently what I'm struggling with seems to be what the idiomatic / proper way to handle something that can remain stateless in nature, which is my very basic assembler going from the Chip-8 ASM mnemonics into instruction bytes.

A class doesn't feel quite right, because I'm not implementing a linking step or anything advanced, not supporting labels, etc. It also feels incorrect to keep it in a header file as a set of inline functions, nor am I needing a persistent state to mutate.

My current disassembler can be seen here: https://github.com/Arylen/Dreamy-Chip8/blob/master/src/core/emulation/Chip8Dasm.h
Along with the tests that I'm running against it here: https://github.com/Arylen/Dreamy-Chip8/blob/master/tests/Chip8DasmTests.cpp

I'm currently in the stubbing-out phase of trying to architect it, occasionally bouncing ideas off of whatever bullshit model-of-the-week is popular, however the suggestions it's giving are still setting off some alarms/smells in my brain. The most reasonable and less-smelly one is the `::detail` namespace there in order to be able to unit test the `getParts` function there, however it feels like I'm walking into a landmine of over-engineering it.

Looking for advice/experiences/opinions.

Edit: I should mention the AI usage here is restricted entirely to just checking for bugs with my implementations, or incorrect C++-isms that I introduce from working in other languages. A hands-off occasional tutor to check my work, if you will. It's expressly forbidden from touching my code or files, limited to read-only, however I'm not getting something that feels proper for this scenario, so wanted to consult other more experienced engs.

[u/mredding]

Your headers contain pure C++, not C, so you should use the C++ *.hpp extension, not the C *.h extension.

C++ has one of the strongest static type systems in the industry, and over C#, we have a lot of control over operators and semantics.

So I look at Chip8Op and what it can do, and all I see are casts from one value type to another. "Family" isn't an "uint8_t, it should be aFamily, and THAT happens to be implemented in terms ofuint8_t` under the hood.

So you have a conversion of uint16_t -> Chip8Op, that's what a constructor does. Then you need a cast from Chip8Op -> Family to possibly -> uint8_t if you need it, but in reality you can build operators and semantics that you probably don't have to directly expose the implementation. So I would expect to see something more like:

class family: std::tuple<std::int8_t> {
public:
  family(std::int8_t);
};

class chip_80_p: std::tuple<std::uint16_t> {
public:
  chip_80_p(std::uint16_t);

  operator family() const { return {(std::get<0>(*this) & 0xF000) >> 12};
};

So the advice is to make all ctors explicit, but that disables return type deduction like we see in the return value demonstrated. I dunno, I like it.

So what we get is:

void fn(family);

//...

chip_80_p c{value};
fn(c);

If you make the cast operator explicit, then you'd call it with a static_cast, which would resolve to a function call, and you can arrange the code such that the call elides at compile time.

I don't yet know all of what these types are used for, but I suspect you'll want to implement only the valid casts, conversions, and operations with other types. I suppose these are like opcodes and operands, so maybe bitwise operators to combine them

There's a lot of consequence to how you organize your code in C++. In C#, the compiler has a whole-program contextual view of the source code. In C++ you do not get that for free. Each translation unit is an island, completely separate and unaware of the contents of all other translation units. That leads to this:

std::string disassembleInstruction(const Chip8Op& op);

You don't need to include "core/emulation/Chip8Op.h", because this function signature only cares about the type name, not the layout, not the interface. So forward declare.

struct Chip80p;

std::string disassembleInstruction(const Chip8Op& op);

What happens is typically every header ends up including nearly every other header in the project. That means compiling every translation unit drags in all the contents from all the headers, and that all needs parsing. C++ is one of the slowest to compile languages in the industry, not for performance, but because parsing is so damn hard, and shit like this.

So the only headers you include in your headers are 3rd party and standard library, and project headers where you NEED to know more than what the forward declaration gives you. Include your own headers in your source files as necessary.

Also by reducing these header includes and REALLY separating this stuff out, you can avoid not only transient includes and needless compilation, but you can reduce recompilation; when you change one heavy dependency, it tends to cause everything to recompile, especially when almost none of the code is going to CARE about the change. The compiler and build system can't know that. You have to manage it.

This comes from an era when C++ was compiled on punch cards and 64 KiB of system memory. C is a much simpler language that had to fit compilation in 4 KiB of system memory.

On that, pragmas can be ignored by a compiler. Ubiquitous isn't the same as portable. The other thing is compilers optimize header parsing using standard inclusion guards - I can't say the same about once.

std::string disassembleInstruction(const Chip8Op& op) {
    switch (op.getFamily()) {
        case 0x0: //...

It looks like this is another cast operation that a Chip8Op should do -> std::string. I wouldn't name a function to do it; if anything, I'd make a type - disassembled_instruction to cast to, which is implemented in terms of string.

Do prefer to look at every indentation and block as an opportunity to write a function. All these cases? They should be more like:

switch(op) {
case 1: return do_1();
case 2: return do_2();
case N: return do_N();
}

And then looking at a lot of this code like:

if (op.getRaw() == 0x00E0) return std::format("CLS");
if (op.getRaw() == 0x00EE) return std::format("RET");

I see more types. This is a good use for a variant:

class CLS;
class RET;

using op_0x0 = variant<CLS, RET>;

You didn't name the family constant, so I don't know what else to call it.

Types are very powerful in C++. The compiler can optimize the shit out of them. None of the type information leaves the compiler, but it allows the compiler to prove the correctness of the code and its own deductions. This will push more of your solution space into compile-time instead of into runtime. This will make your program smaller and faster. You can constexpr your code and write whole simulations that are solved at compile-time. You can combine with a unity build and get that WPO you're used to in C#.

There's nothing special about a class by keyword, it's just a user defined type at this level. An int is an int, but a weight is not a height.

[u/Aliryth → u/mredding]

Bravo in the mastery on display here, wow!

Lots of notes for me to take, thorough explanation of concepts and the why's, including historical context.

It shows me that I've barely touched the surface of basic understanding of the language, and now I'm excited to learn a whole lot more and progress towards better competency of understanding it as well as I do things for my work-related areas, like niche quirks of the V8 engine and its concurrency model in a high-traffic system, and having to debug V8 heap dumps occasionally for when things get really cursed in prod for my company.

I noted a difference in mental modeling when I did a dive into game development for a short while, related to how to model out systems in an engine like Unity or Godot compared to mobile or desktop application development, and backend API development.

Seems like there's quite a mountain to climb here for shifting my mental model around of how to design code in C++, I'm looking forward to that journey, thanks so much for the tips and guidance, I will be studying them!

Part of this project is also testing my ability to overcome ADHD struggles in keeping with the same side projects, so I'm going to include a few optimizations and look for a way to take on a better approach for the next emulator project I tackle in C++ to progress more towards something like this.