stella-emu / stella

A multi-platform Atari 2600 Emulator
https://stella-emu.github.io
GNU General Public License v2.0
618 stars 112 forks source link

Fully convert C-style classes to C++ classes #241

Open sa666666 opened 7 years ago

sa666666 commented 7 years ago

Just a note to remind myself of this. A number of 'classes' are classes only in name; they are essentially C-style code put in a C++ class. This only applies to code I've written, not to stuff from other places (libpng, zlib, etc).

These classes contain several things that need to be fixed. Primarily, they're using malloc/free, raw pointers, and old style casts. For now, we need to convert the following (in no particular order):

This will be expanded upon as I encounter more such classes.

sa666666 commented 6 years ago

I've determined that changes to the UI code are much too invasive, and would require a large re-write. If I would ever attempt that, I'd rather just move to another toolkit entirely. So I will leave the UI code using raw pointers. Various checks by Coverity and valgrind show no leaks, so it's really a waste of time to work on this part any further.

sa666666 commented 6 years ago

ZipHandler is already mostly re-written; I will commit after more testing.

YaccParser needs a complete re-write, probably after 5.2 release.

g012 commented 6 years ago

I'm curious about the rationale. Why not fully convert them to C ?

Using malloc / free is in no way related to the language, you can create a memory allocator in C even more versatile than in C++, which doesn't support some C constructs, eg. struct overallocation.

Same goes with raw pointers, nothing stops you from wrapping them into structs to get compile time type safety. A better approach would even be to move them to handles or typed indices whenever there are more than one instance, and C++ is of no special help here.

Finally, any C compiler warns about seemingly incompatible cast types just like static_cast in C++.

sa666666 commented 6 years ago

The rationale for not touching the UI stuff would be the amount of work involved. I underestimated it, and after spending about 6-7 hours working on it, came to the conclusion that it is too much work for little improvement, especially since the current code works, and doesn't leak.

ZipHandler is something I wanted to rewrite anyway, and is already mostly done.

YaccParser really needs to be C++, and was written in a time when yacc/bison didn't have mature C++ support. Now that it does, it makes sense to use it.

Overall, my main goal is to move as much of the C-specific code as possible to C++. The UI stuff is already C++; I just wanted to make it use smart pointers. But it isn't an absolute requirement. The other two are directly C, and use malloc/free, which is not recommended to be used together with new/delete from C++. So I will concentrate on those.

sa666666 commented 6 years ago

And when I say 'move to C++', of course I mean to move to the canonical 'C++11 way' of doing things. So where possible, not using new/delete at all, but instead using smart pointers. Having/using raw pointers is not too bad, but it would be nice to fix. But definitely moving away from C-style memory management (no malloc/free/sprintf/etc).

sa666666 commented 6 years ago

ZipHandler is now done. Also fixes a compile warning in VS.