skmp / reicast-emulator

Reicast was a multiplatform Sega Dreamcast emulator
https://reicast.emudev.org
Other
1.1k stars 344 forks source link

Restructure the codebase to not use globals #1140

Open skmp opened 6 years ago

skmp commented 6 years ago

Currently, the codebase uses globals for any kind of state. While this is easy to read and fast for armv7/x86, it gets more of a pain with aarch64/x64, and also makes testing much more involved

Redream used the 'c-with-context-param' approach to solve this. Doing this makes initialization much more explicit, and thus, verbose. There is a bit of overhead to pay, though likely to not matter enough perf wise.

I'm not 100% convinced we should go this way, but it sounds like a good idea. I'm also not sure that using C structs w/ functions is better than C++ classes. Thoughts?

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/57533603-restructure-the-codebase-to-not-use-globals?utm_campaign=plugin&utm_content=tracker%2F500311&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F500311&utm_medium=issues&utm_source=github).
psy-fidelious commented 6 years ago

Inlining low-level performance critical functions will definitely help there. Otherwise, I expect the overhead will be negligible. classes vs structs is (for me) a matter of preference with a few caveats: Classes do force you to include other headers in your class header file when you use composition instead of aggregation. In C you can avoid this by using a pimple idiom, keep the definition in your .c/.cpp file and client code never needs to know what your structs are composed of. Needless to say, this will also help with compile times.

pold500 commented 6 years ago

Can you please assign this task to me? I'm looking to refactor the codebase to get rid of global vars.

baka0815 commented 6 years ago

You're free to start and just create a PR with your changes.

skmp commented 6 years ago

@pold500 what would be your plans? Are you on the discord chat? (JayD?)

luserx0 commented 6 years ago

@pold500 @dmiller423 @skmp @AbandonedCart Any ideas?

pold500 commented 6 years ago

@psy-fidelious for your knowledge, pimpl idiom is successfully used in C++ too. via reference to a class member or unique_ptr. @luserx0 my idea is to put everything in classes through composition. so we don't rely on externs which are placed in random places of code. as much as I hate oop and love functional programming, organizing everything under classes seems like a nice idea to me.

dmiller423 commented 6 years ago

It's not a bad idea, but it does nothing for us at this time .. The entire emulator needs an overhaul and refactor, why just wrap globals and still use mostly C-style code everywhere else? Unless we do the rewrite, I see no real benefit. We'd just end up w. a global Globals.var or .getVar(). , or have to pass a context, or make a singleton and query it to get its instance and do Globals::instance()->var ^ ...

I'm always open to suggestions, but again i see little real improment here except to say: Well now we don't have global vars because they are bad mmmkay?

AbandonedCart commented 6 years ago

@dmiller423 makes a very valid point. Partial rewrites just to disguise the core issue are something you do to look busy.

psy-fidelious commented 6 years ago

@pold500 the difference is that in C you can hide the contents of the struct itself in a C file, only exposing the interface in the header (struct declaration + functions operating on struct pointer). In C++ if one wants to expose the interface (methods) they need to also expose the contents of the class, which means include headers for any contained classes. You can choose to reference or allocate those on the heap, but that's not always desirable. Not that it's really important for this thread anyway, signing off.

skmp commented 6 years ago

Added to the Mid Term Goals milestone. @pold500 if you have ideas for this, can you write a small proposal here and/or make a small diff / proof of concept to illustrate how things would look?

pold500 commented 6 years ago

@psy-fidelious Please read some article about pimpl idiom in C++. Short example, in C++:

class PimplClass;

class ClassUsingPimplClass
{
....
private:
    PimplClass& pimplClass; //you don't include any headers for PimplClass.
//or with the unique ptr:
   std::unique_ptr<PimplClass> m_pImpl;
}

@skmp ok, I'll make an experimental pr.

psy-fidelious commented 6 years ago

@pold500 forgive my lack of knowledge I will promptly go and educate myself on the matter further.

pold500 commented 6 years ago

@psy-fidelious It's OK. My apologies for my selfish and entitled tone. I rephrased my comment.

skmp commented 4 years ago

I'm taking over this now as no progress has been made.

See #1760 #1764 #1765 #1766 #1769 #1660 and others.

@pold500 are you still willing to lend a hand?

pold500 commented 4 years ago

@skmp sure!