Opened 8 years ago
Last modified 7 months ago
#4211 new defect
Remove references to globals
Reported by: | elexis | Owned by: | |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Backlog |
Component: | Core engine | Keywords: | |
Cc: | Patch: |
Description (last modified by )
Globals should be avoided in C++, since they have to be checked for initialization in most cases. Instead a static GetFoo function could be used that does that check and returns the instance.
One example would be g_Game
being used in various places of source/simulation/
.
Change History (14)
comment:1 by , 7 years ago
comment:3 by , 6 years ago
Milestone: | Backlog → Work In Progress |
---|---|
Owner: | set to |
Patch: | → Phab:D1670 |
comment:4 by , 5 years ago
Milestone: | Work In Progress → Backlog |
---|---|
Owner: | removed |
comment:5 by , 16 months ago
Patch: | Phab:D1670 |
---|
There is no consensus on whether changing globals to static functions is needed/wanted.
The diff Phab:D1670 was abandoned for this reason.
comment:6 by , 16 months ago
[11:39:29] <elexis1> #4211 this one seriously lacks the information on how the globals should be removed. its not a choice between using a global g_Game and using a global static GetGame. the game reference should be stored in a LOCAL variable m_Game and either be passed in the constructor or using a setter if necessary [11:40:18] <elexis1> vladislav knows fully well that it should be done like that and Im glad he had this ticket reopen, but Itms and Angen/Silier dont understand, so probably future readers wont understand either, and Vladislav didnt post on the ticket how it should be done [11:40:53] <elexis1> (I didnt understand at the time of writing the ticket either) [11:41:44] <elexis1> (23:08:28) vladislav: Kind of dependency injection. [11:41:44] <elexis1> (23:08:28) elexis: https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/gui/session/session.js#L287 [11:41:44] <elexis1> (23:09:29) vladislav: Yeah
comment:7 by , 16 months ago
Generally we have at least two basic ways to pass some dependencies:
1. We might store it locally:
class CCamera { public: CCamera(CTerrain& terrain, ...) : m_Terrain(terrain) {} void Update() { const float height = m_Terrain.GetHeight(...); } private: // Might a pointer if it can be nullptr or might be a copy // if its size isn't critical in this context. CTerrain& m_Terrain; };
2. We might pass it via a function argument:
class CCamera { public: // The terrain argument might be a pointer if can be nullptr. void Update(CTerrain& terrain) { const float height = terrain->GetHeight(...); } };
Depending on a) a context, on b) a dependency lifetime and c) how often a dependency might change we might choose one or another. For example, if the lifetime is guaranteed and we need to use the dependency in mostly every method then storing it as a member is a default choice.
follow-ups: 9 10 comment:8 by , 16 months ago
Should this ticket be closed as wontfix or repurposed to "C++ Reduce globals"?
Most Globals are used because it's not trivial to bind arguments to JS-interface functions (JS_GetContextPrivate
should be used). Another reason is that it's not possible to bind arguments to an InHandler
(std::function might be used).
About the lifetime issue when a reference is stored in a class. A bit saver way would be to not take the parameter by reference but take a wrapper to "force" the caller to think about it:
Type obj{StoresReference{param}};
when a reference is stored and Type obj{param};
if the parameter is only used in the constructor. I never saw that convention in other codebases though.
comment:9 by , 16 months ago
Replying to phosit:
Should this ticket be closed as wontfix
It can't be closed until it's fixed.
or repurposed to "C++ Reduce globals"?
We need to remove globals not only in C++ but also in JS. Not sure if it's worth to make a different ticket. Because the current ticket is more about design.
comment:10 by , 12 months ago
Replying to phosit:
Most Globals are used because it's not trivial to bind arguments to JS-interface functions (
JS_GetContextPrivate
should be used).
The ScriptInterface
is not typesafe: To set callback data we have to convert a pointer to the data to void*
and call SetCallbackData
. To get the data back ObjectFromCBData<T>
is called. If T
is not the type previously set there is no warning. There is no way to check if the internal void*
actually points to a T
.
I propose to make ScriptInterface
a template. SetCallbackData
(I don't think this is nessesary the callback data should be set when constructing the ScriptInterface
) and ObjectFromCBData
would only work with the template type: The signature would look like this
template<typename T> T& ScriptInterface<T>::ObjectFromCBData()
.
Another issue is when the object where the void*
is pointing at is destructed (e.g. goes out of scope). That can be solved by placing that object inside the ScriptInterface
.
comment:12 by , 12 months ago
There are a few things to look at on this topic:
- Some globals, like the C++ g_Game global, are 'dependencies of everything'.
- Some globals, like the Simulation, are dependencies of most Sim object.
- Some globals might just be dependencies of specific classes / files.
- Some globals are basically 'always on', like Config, some aren't, like Game. It's worse in JS.
There are a few different approaches to improving the situation IMO:
- In C++, we can differentiate read/write access. That's already an improvement IMO.
- In some cases, we can use the approach we use for Sim components: they store a
SimContext
object, which is itself pointing to a number of 'global' objects. This is useful, as we can run several instances of the simulation in parallel, and it provides a neat place to do dependency injection. This approach could be extended to a GameContext, though most likely it would still be kinda useful to have a global for this for now. - Of course in some tight coupling cases storing it as a member variable in C++ makes more sense.
- Passing as arguments in some cases also makes more sense.
For JS code, I think we should just switch to modules. They essentially fix the problem.
The ticket was created following #3991
See
2016-09-12-QuakeNet-#0ad-dev.log
: