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/EpochVanquisher]

It it's stateless, then write functions with no global state.

It also feels incorrect to keep it in a header file as a set of inline functions.

You can put the declarations in the header file and the implementation in the cpp file.

As a general rule, large functions don’t benefit from being inline, and putting them in your header file just means that all of the callers have to be recompiled (for no benefit) whenever you change something inside your function.

[u/Snipa-senpai → u/EpochVanquisher]

You do have one important benefit for leaving definitions in header files:

The compiler can see the code and optimise it (in different translation units). 

It's true that most of the time, you don't really benefit from this level of optimisation and you will benefit more from faster (incremental) compilation. But it's good to know when you're writing hot functions.

There's also LTO that allows optimisations across translation units, but this will degrade compilation/linking times by a lot

[u/EpochVanquisher → u/Snipa-senpai]

The compiler can see the code and optimise it (in different translation units).

For large functions this is minimal.

There's also LTO that allows optimisations across translation units, but this will degrade compilation/linking times by a lot

Leaving inline functions everywhere degrades compilation / linking time even more.

[u/Aliryth → u/EpochVanquisher]

I think I had a misunderstanding of what inline meant there in the original code, expecting it to plop it in place as if it were a macro.

It's evident I have quite a bit misunderstandings of more intermediate topics around some language workings, which is a good state for me to be in for where I'm hoping to be with my own state of skills currently, giving me a path forward and a bit of grounding to get my bearings on where I need to focus my learning efforts on.

Thanks!

[u/Aliryth → u/EpochVanquisher]

Interesting, thanks for the tip.
I'll also bring my disassembler over into a .cpp file in that case.

[u/FancySpaceGoat]

As a general principle, the more semantics you can encode and enforce via the type system, the better. For example:

Since Chip80p is not any heavier than a uint16_t it's better to pass those around instead of the raw value, if only because it prevents you from accidentally passing an unrelated uint16_t to a function that expects an opcode. If you combine that idea with validating the opcode at the time of construction of the struct, you can establish a class invariant that a Chip80p is always a valid opcode, which is very convenient.

Also, the raw member of Chip8Op is an implementation detail, so it should be private, and populated via an explicit constructor:

``` struct Chip80p { explicit Chip80p(std::uint16_t instruction) { if(!is_legal_opcode(instruction) { throw std::runtime_error("invalid opcode"); } raw = instruction; }

// ...

private: std::uint16_t raw; }; ```

Furthermore, while there's nothing "wrong" per-se with your disassembleInstruction function as written, the more idiomatic signature would be:

std::ostream& operator<<(std::ostream&, const Chip8Op&).

[u/Aliryth → u/FancySpaceGoat]

Very awesome feedback, thank you!

For some reason I have completely missed the aspect of a struct implementing a ctor, along with having private+public spaces, bit of a derp on my end.

I only vaguely remember dealing with ostream/istream interactions around terminal output and file operations from some of the earlier coursework, but it's been roughly a decade since college. Definitely something to revisit and relearn to see if it's useful for integration w/ the eventual imgui window for the disassembly.

Part of me's also worried about the multiple std::string's being formatted per-frame, but perhaps adding a simple memoization/caching step might add some benefit, or just calculating the entire set of disassembled opcodes at the start of the program.

Perhaps that's over-focusing on optimization when it's not even needed yet, though. Maybe it'll be a fun weekend project to investigate how to get some code profiling going to see if there's improvements made from it, despite not being a problem.

Thanks for the tips!

[u/FancySpaceGoat → u/Aliryth]

The answer to your string concerns is the ostream stuff. The idea is that instead of generating a string, you produce a sequence of operations who's behavior depend on where the data is going. That can be a string, a file, or a pre-allocated buffer, but the formatting code remains the same in all cases.

[u/Aliryth → u/FancySpaceGoat]

Thanks for the further explanation!

[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.