5 ms·
This is a cool project with nice, simple code. However, the submission's title (perhaps counter to the README) makes it seem like a good example of C++11, and I
by spoondan 10y ago
This is a cool project with nice, simple code. However, the submission's title (perhaps counter to the README) makes it seem like a good example of C++11, and I would caution against using it that way. From a cursory examination, and with all due respect to the author, this code is more classical C++ with a few uses of C++11 features than it is representative of how modern C++ (at the time of C++11's publication) should be written. There are a few areas where the code could be updated: adding tests, avoiding manual memory management, making it so objects cannot be created in invalid states that would cause crashes, const correctness, etc.
By way of explanation, I'll focus on just the issue of manual memory management. Simplifying a bit, consider this part of the C64 class's implementation:
C64::C64()
{
cpu_ = new Cpu();
mem_ = new Memory();
cia1_ = new Cia1();
...
cpu_->memory(mem_);
cpu_->reset();
...
cia1_->cpu(cpu_);
...
mem_->cia1(cia1_);
...
}
C64::~C64()
{
delete cpu_;
delete mem_;
delete cia1_;
...
}
The contract in this code is that a C64 owns the chips, the chips are born when the C64 is instantiated, and when the C64 dies, the chips all die along with it. There are two approaches for making this contract explicit, one is to simply not use pointers and references at all:
Cpu cpu_;
Memory memory_;
Cia1 cia1_;
But this does mean we can't use polymorphism within the C64 class, we don't have nullability, and we can't replace the instances. The code doesn't leverage any of these, so that may be an appropriate choice. However, if we need any of those things, we can instead use the appropriate smart pointer:
std::unique_ptr<Cpu> cpu_;
std::unique_ptr<Memory> memory_;
std::unique_ptr<Cia1> cia1_;
In either case, C64's destructor is no longer necessary. The other classes still take raw pointers (e.g., `Cpu*`) because they do not own the objects they receive and do not dictate lifetimes.
It's worth mentioning that many people would use `std::shared_ptr` here instead, but I think that's abuse that leads you towards circular references (memory leaks): the Cpu doesn't own its Memory, they just have their lifetimes managed exterior to them. This would actually come up with the existing design (if shared ownership were used), since the object dependency graph (which is perhaps another issue) has several circular dependencies.
- nibnib 10y agoSlightly off-topic, but could you recommend any books/articles/talks/resources on modern C++? I learnt the language in the early 2000s and haven't used it in years. If I picked up a C++ project today I would write code exactly like the above.
- byuu 10y agoOn this topic, I'd like to get the advice of some HN C++ gurus. In my own emulator (higan), I have a namespace for each emulated system, and each component is its own unique object. Example: namespace GameBoyAdvance { CPU cpu; PPU ppu; Cartridge cartridge; ... } The obvious downside is that they aren't instantiable. You can only have one instance per process. Generally this isn't a problem for an emulator, but it's still something I don't like about my code. The obvious "proper" way would be something like: class GameBoyAdvance { CPU cpu; PPU ppu; ... }; But there's a reason I don't do this: all of the chips need to communicate with one another. So PPU::scanline() may call CPU::raiseInterrupt(Interrupt::VerticalBlank) for example. So you think something like: class GameBoyAdvance { CPU cpu{this}; //hooray C++11, this would be PPU ppu{this}; //much nastier in C++98 ... }; And now your code goes from this: void PPU::scanline() { cpu.raiseInterrupt(Interrupt::VerticalBlank); } To this: void PPU::scanline() { gameBoyAdvance->cpu.raiseInterrupt(Interrupt::VerticalBlank); } Which aside from being a lot of extra typing (seriously, the chips in emulated hardware communicate all the time, all over the place), is also quite a lot slower. The former can basically get inlined 100%. The compiler knows the exact address of the CPU object, because there's only one. The latter has to go through a pointer to get the address. Even though it's always a constant under actual emulation, the code basically has to account for the fact that you could change the pointer at any time. Even if you tried binding it as a reference, eg CPU(GameBoyAdvance&), the performance penalty remains. Long ago, I wrote some #define wrappers so I could say "cpu().raiseInterrupt", and depending on compilation flags, it would use CPU cpu; or CPU* cpu; and the results were dramatic: at least a 15% loss of performance just by using the pointer version. I'd really like the benefits of the latter design, but without the added red tape and performance penalties. But I can't think of any way to pull it off. I really can't afford the performance penalties, as I use 100% of top-end CPUs to try to emulate every hardware nuance possible already. Am I doing the right thing, or is there a better way?
- CJefferson 10y agoIt's a bit horrible and unsafe, but you can use offsetof and some nasty pointer casting to get from a member of a class back to the class. I've done this, and it compiles to extremely efficient code. You can even template a class on the value of offsetof, so you can use the same class as multiple members. I'm on my phone, so no code examples (sorry). If you want one, reply and I'll write one when I get to a computer!
- kazinator 10y agoYou are nearly completely right. The one good thing about the new/delete approach is that it keeps the master object small, since it's just a farm of pointers. No matter how you define a C64 instance, most of the storage comes from the heap. I once ran into an issue where declaring a class on the stack was actually crashing the program (running in a small-ish embedded system). The quick and dirty fix was to new/delete the entire object. But when that is out in the open, you expose potential leaks; you need a smart pointer to manage that. Making some of the contents dynamic (so that the class is effectively a smart pointer) is another option.