Page MenuHomeWildfire Games
Differential D1670

Singleton g_Game and g_SoundManager
AbandonedPublic
Actions

Authored by Silier on Nov 9 2018, 7:31 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#4211
Summary

There should be no globals in C++ and if, they should constant and/or use getters and setters. So following #4211 I decided to remove globals from the code.
I try to keep revision small, so maybe it will be done in several revisions.

As far removed globals:

  • g_Game
  • g_SoundManager
Test Plan

Run game and watch for potential problems (should be none).

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6439
Build 10668: Vulcan BuildJenkins
Build 10665: arc lint + arc unit

Event Timeline

Silier created this revision.Nov 9 2018, 7:31 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package, Restricted Owners Package.Nov 9 2018, 7:31 PM
Vulcan added a subscriber: Vulcan.Nov 9 2018, 7:36 PM

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
|  33| #include·"lib/file/vfs/vfs_path.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
|  33| #include·"lib/file/vfs/vfs_path.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
|  33| #include·"lib/file/vfs/vfs_path.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/View.cpp
|  33| #include·"lib/utf8.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/View.cpp
|  33| #include·"lib/utf8.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'CONFIG2_GLES'.

source/tools/atlas/GameInterface/View.cpp
|  33| #include·"lib/utf8.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/View.cpp
|  33| #include·"lib/utf8.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Misc.cpp
|  33| »   case·0:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Misc.cpp
|  33| »   case·0:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Misc.cpp
|  33| »   case·0:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp
|  33| #include·"maths/Vector2D.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp
|  33| #include·"maths/Vector2D.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp
|  33| #include·"maths/Vector2D.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/CameraCtrlHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/CameraCtrlHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/CameraCtrlHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/TerrainHandlers.cpp
|  33| #include·"simulation2/components/ICmpPathfinder.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/TerrainHandlers.cpp
|  33| #include·"simulation2/components/ICmpPathfinder.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/TerrainHandlers.cpp
|  33| #include·"simulation2/components/ICmpPathfinder.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/EnvironmentHandlers.cpp
|  33| #include·"renderer/WaterManager.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/EnvironmentHandlers.cpp
|  33| #include·"renderer/WaterManager.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/EnvironmentHandlers.cpp
|  33| #include·"renderer/WaterManager.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/PlayerHandlers.cpp
|  33| QUERYHANDLER(GetPlayerDefaults)
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/PlayerHandlers.cpp
|  33| QUERYHANDLER(GetPlayerDefaults)
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/PlayerHandlers.cpp
|  33| QUERYHANDLER(GetPlayerDefaults)
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...

Link to build: https://jenkins.wildfiregames.com/job/differential/773/

Harbormaster failed remote builds in B6439: Diff 6974!Nov 9 2018, 7:37 PM
vladislavbelov added a subscriber: vladislavbelov.EditedNov 9 2018, 7:42 PM

Function were declarated by CC, but used with a wrong first letter.

Also I'm not sure, because g_Game is shorter than Game::GetInstance, but the meaning is the same - singleton. Semantic wasn't changed, only accessing. I'd say:

#define g_Game Game::GetInstance()

As it's done for g_ConfigDB.

source/ps/Game.h
84

You need to use the Singleton<Game> deriving instead on the own implementation.

Silier planned changes to this revision.Nov 9 2018, 7:42 PM

Renaming did not applied another files

Silier updated this revision to Diff 6976.Nov 9 2018, 8:17 PM

Fix Capital Letters

Vulcan added a comment.Nov 9 2018, 8:39 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/View.cpp
|  33| #include·"lib/utf8.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/View.cpp
|  33| #include·"lib/utf8.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'CONFIG2_GLES'.

source/tools/atlas/GameInterface/View.cpp
|  33| #include·"lib/utf8.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/View.cpp
|  33| #include·"lib/utf8.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
|  33| #include·"lib/file/vfs/vfs_path.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
|  33| #include·"lib/file/vfs/vfs_path.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
|  33| #include·"lib/file/vfs/vfs_path.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/CameraCtrlHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/CameraCtrlHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/CameraCtrlHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/PlayerHandlers.cpp
|  33| QUERYHANDLER(GetPlayerDefaults)
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/PlayerHandlers.cpp
|  33| QUERYHANDLER(GetPlayerDefaults)
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/PlayerHandlers.cpp
|  33| QUERYHANDLER(GetPlayerDefaults)
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp
|  33| #include·"maths/Vector2D.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp
|  33| #include·"maths/Vector2D.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp
|  33| #include·"maths/Vector2D.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/TerrainHandlers.cpp
|  33| #include·"simulation2/components/ICmpPathfinder.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/TerrainHandlers.cpp
|  33| #include·"simulation2/components/ICmpPathfinder.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/TerrainHandlers.cpp
|  33| #include·"simulation2/components/ICmpPathfinder.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Misc.cpp
|  33| »   case·0:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Misc.cpp
|  33| »   case·0:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Misc.cpp
|  33| »   case·0:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/EnvironmentHandlers.cpp
|  33| #include·"renderer/WaterManager.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/EnvironmentHandlers.cpp
|  33| #include·"renderer/WaterManager.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/EnvironmentHandlers.cpp
|  33| #include·"renderer/WaterManager.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...

Link to build: https://jenkins.wildfiregames.com/job/differential/774/

Harbormaster completed remote builds in B6441: Diff 6976.Nov 9 2018, 8:39 PM
lyv added a subscriber: lyv.Nov 10 2018, 5:12 AM

Aside from these two things, I also thought nullptr was preffered.

source/ps/Game.cpp
75

Redundant check, no?
Not sure, but I think even CC discourages it.

78

CC says to use “SAFE_DELETE” which does exactly what these two lines do.

lyv added inline comments.Nov 10 2018, 5:18 AM
source/ps/Game.cpp
68

Even more redundant since there is a check in DeInitGame

Silier marked 3 inline comments as done.Nov 10 2018, 9:40 AM
Silier added inline comments.
source/ps/Game.cpp
68

right

75

Trying to delete Null would cause program to fail. So check here or before every calling. I prefer here.

source/ps/Game.h
84

I was not aware about this option. But can it be used if g_Game is initialised with different parameters and deleted in several places?

lyv added inline comments.Nov 10 2018, 9:47 AM
source/ps/Game.cpp
75

delete does the check. And its harmless. And it was in the CC after all. Relevant part copy pasted.

  • Don't do "if (p) delete p;". (That's redundant since "delete NULL;" is safe and does nothing.)
  • If deleting a pointer, and it's not in a destructor, and it's not being immediately assigned a new value, use "SAFE_DELETE(p)" (which is equivalent to "delete p; p = NULL;") to avoid dangling pointers to deleted memory.
vladislavbelov added inline comments.Nov 10 2018, 10:37 AM
source/ps/Game.cpp
60

nullptr here and in other places.

63

The current version isn't safer than the old one, because it doesn't any check here.

66

No need to duplicate default values.

source/ps/Game.h
83

Put the private part under the public one.

Also I'd prefer to try use std::unique_ptr instead of raw pointers for a more safe control.

84

Yes, it's possible, something like this:

class CGame : public Singleton<CGame> {};
// Before usage
new CGame(..., ...);
// Usage
CGame::GetSingleton().Call...;
// After usage
delete CGame::GetSingletonPtr();
source/soundmanager/ISoundManager.h
30

Put it under the public section.

Silier marked 4 inline comments as done.Nov 10 2018, 11:00 AM
Silier added inline comments.
source/ps/Game.cpp
63

actually I had here default constructor if called upon null and wanted to remove all checks in code, but I was not sure yet if it safe to do.

66

I found this one in CC

source/ps/Game.h
83

idea was to enforce getter

source/ps/scripting/JSInterface_SavedGame.cpp
76

probably should uncomment this back

Silier added a comment.Nov 10 2018, 11:18 AM

Or another idea for geters upon pointers like g_Game is that if called upon null, throw exception and remove unneded checks in code. So getGame should never be called when is not initialised.

Not good idea to do this with soundmanager because there is null value like part of strategy, but could do it.

Stan added a subscriber: Stan.Nov 10 2018, 11:23 AM
Stan added inline comments.
source/ps/Game.cpp
79

nullptr

83

nullptr

source/soundmanager/SoundManager.cpp
182

nullptr

204

nullptr

source/tools/atlas/GameInterface/SimState.cpp
32

nullptr

Silier marked an inline comment as done.Nov 10 2018, 11:35 AM
source/ps/Game.cpp
83

ok, so looks like i am going to do separate diff before this one and replace all NULL with nullptr among whole solution to get rid of it.

vladislavbelov added inline comments.Nov 10 2018, 11:35 AM
source/ps/Game.cpp
66

But it's not a requirement, only can be useful.

source/ps/Game.h
83

It's not related to. I'm talking about storing, you're always able to return raw pointer as unique_ptr.get().

Silier marked an inline comment as done.Nov 10 2018, 11:42 AM
Silier added inline comments.
source/ps/Game.h
83

ok I missunderstood put the private under public

Stan requested changes to this revision.Apr 19 2019, 6:02 PM
Stan added reviewers: Restricted Owners Package, causative.

Needs to be rebased.

This revision now requires changes to proceed.Apr 19 2019, 6:02 PM
Stan edited reviewers, added: Restricted Owners Package; removed: causative.Apr 19 2019, 6:03 PM
Stan edited reviewers, added: wraitii; removed: Stan, Restricted Owners Package.Jun 20 2019, 3:42 PM

I think it would be nice to have this.

This revision now requires review to proceed.Jun 20 2019, 3:42 PM
wraitii added a comment.Jun 20 2019, 4:09 PM

I kind of fail to see the point of replacing a constant with a function call. It's more code to read and provides us little.

There's two kinds of code:

  • code that can work without the global.
  • code that can't.

The formed should check for existence, a nullable global works just fine imo.
The code that probably shouldn't be implicitly initialising a g_Game global either. Failing hard and fast seems fine.

There's two other kinds of code:

  • code that needs a mutable global (bad)
  • code that needs read-only access (OK)

For the former, we should have a const reference to the mutable global.
For the latter, just use the mutable global.

e.g.

Game* g_MutableGame = nullptr;
const Game*& g_Game = g_MutableGame;

The code that calls the mutable global automatically becomes suspect.

Krinkle added a subscriber: Krinkle.Jun 20 2019, 4:38 PM
wraitii requested changes to this revision.Aug 23 2019, 9:49 AM

I don't believe this change is worth committing as it stands, as Vlad said this only changes semantics (arguably making them worse since the new code is longer).
It's ever so slightly annoying for svn-blame to change so many lines, and to me the cost vs win calculation is negative here.

I think a more worthwhile change would be, as described above, to have clearer distinction between "const" access to these globals and non-const access. This can be done without changing any LoC by declaring a g_MutableGame and g_Game as a const reference to that - then you only need to change all code that actually modifies g_Game, and we might be able to notice oddities.

This revision now requires changes to proceed.Aug 23 2019, 9:49 AM
elexis added a comment.EditedAug 23 2019, 10:47 AM

I don't believe this change is worth committing as it stands, as Vlad said this only changes semantics (arguably making them worse since the new code is longer).

It doesnt change semantics, does it? Only syntax.

The patch doesn't change that they are still globals.
As far as I see semantics should be changed to avoid the globals, otherwise there is not much gain, it's still a global singleton ref.

The JSInterface ones for example could use the private field probably, unless it's relevant for performance. And then the next step could be to make a JS specific class, there is a ticket somewhere for XmppClient and NetServer/NetClient. But that needs some thought.

The benefit of not having globals is that the code becomes restructured, so that everything is owned properly, separation of concerns, and in some cases more reusability, so that it can be used with different variables.

For example if there was no g_Game global or singleton, one could run multiple games.
Or if there was no g_XmppClient, one could run multiple XmppClients.

Edit: Perhaps g_Game and g_SoundManager are singleton in nature, then the patch wouldn't make it worse for those two.

elexis retitled this revision from Remove references to globals to Singleton g_Game and g_SoundManager.Aug 23 2019, 11:02 AM
wraitii added a comment.Aug 23 2019, 11:12 AM
In D1670#92067, @elexis wrote:

It doesnt change semantics, does it? Only syntax.

Mh, yes.

Edit: Perhaps g_Game and g_SoundManager are singleton in nature, then the patch wouldn't make it worse for those two.

I would say so (though perhaps the secondary simulation hack could be removed ifs e could).
Anyways, the Coding Conventions say:

Prefer global variables over singletons, because then they're not trying to hide their ugliness.

And I find myself agreeing with that and enforcing that here.

Silier abandoned this revision.Nov 17 2019, 6:24 PM

Revision Contents
Changeset List

PathSize
source/
graphics/
8 lines
14 lines
4 lines
2 lines
4 lines
scripting/
42 lines
gui/
24 lines
16 lines
4 lines
32 lines
18 lines
14 lines
network/
scripting/
19 lines
ps/
12 lines
31 lines
GameSetup/
57 lines
30 lines
14 lines
6 lines
14 lines
4 lines
8 lines
scripting/
56 lines
19 lines
renderer/
2 lines
4 lines
2 lines
6 lines
2 lines
2 lines
2 lines
8 lines
simulation2/
components/
4 lines
12 lines
scripting/
30 lines
system/
2 lines
2 lines
soundmanager/
6 lines
33 lines
data/
4 lines
items/
2 lines
8 lines
2 lines
scripting/
26 lines
18 lines
tools/
atlas/
GameInterface/
2 lines
Handlers/
42 lines
38 lines
20 lines
6 lines
58 lines
10 lines
108 lines
6 lines
16 lines
4 lines
6 lines
4 lines
26 lines

Diff 6974

View Options

source/graphics/Camera.cpp

Loading...
View Options

source/graphics/CinemaManager.cpp

Loading...
View Options

source/graphics/GameView.cpp

Loading...
View Options

source/graphics/LOSTexture.cpp

Loading...
View Options

source/graphics/UnitAnimation.cpp

Loading...
View Options

source/graphics/scripting/JSInterface_GameView.cpp

Loading...
View Options

source/gui/CDropDown.cpp

Loading...
View Options

source/gui/CList.cpp

Loading...
View Options

source/gui/COList.cpp

Loading...
View Options

source/gui/IGUIButtonBehavior.cpp

Loading...
View Options

source/gui/MiniMap.cpp

Loading...
View Options

source/main.cpp

Loading...
View Options

source/network/scripting/JSInterface_Network.cpp

Loading...
View Options

source/ps/Game.h

Loading...
View Options

source/ps/Game.cpp

Loading...
View Options

source/ps/GameSetup/GameSetup.cpp

Loading...
View Options

source/ps/Replay.cpp

Loading...
View Options

source/ps/SavedGame.cpp

Loading...
View Options

source/ps/TouchInput.cpp

Loading...
View Options

source/ps/Util.cpp

Loading...
View Options

source/ps/VideoMode.cpp

Loading...
View Options

source/ps/VisualReplay.cpp

Loading...
View Options

source/ps/scripting/JSInterface_Game.cpp

Loading...
View Options

source/ps/scripting/JSInterface_SavedGame.cpp

Loading...
View Options

source/renderer/PatchRData.cpp

Loading...
View Options

source/renderer/PostprocManager.cpp

Loading...
View Options

source/renderer/RenderModifiers.cpp

Loading...
View Options

source/renderer/Renderer.cpp

Loading...
View Options

source/renderer/SkyManager.cpp

Loading...
View Options

source/renderer/TerrainOverlay.cpp

Loading...
View Options

source/renderer/TerrainRenderer.cpp

Loading...
View Options

source/renderer/WaterManager.cpp

Loading...
View Options

source/simulation2/components/CCmpCommandQueue.cpp

Loading...
View Options

source/simulation2/components/CCmpSoundManager.cpp

Loading...
View Options

source/simulation2/scripting/JSInterface_Simulation.cpp

Loading...
View Options

source/simulation2/system/SimContext.h

Loading...
View Options

source/simulation2/system/SimContext.cpp

Loading...
View Options

source/soundmanager/ISoundManager.h

Loading...
View Options

source/soundmanager/SoundManager.cpp

Loading...
View Options

source/soundmanager/data/OggData.cpp

Loading...
View Options

source/soundmanager/items/CBufferItem.cpp

Loading...
View Options

source/soundmanager/items/CSoundBase.cpp

Loading...
View Options

source/soundmanager/items/CStreamItem.cpp

Loading...
View Options

source/soundmanager/scripting/JSInterface_Sound.cpp

Loading...
View Options

source/soundmanager/scripting/SoundGroup.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Brushes.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Handlers/CameraCtrlHandlers.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Handlers/EnvironmentHandlers.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Handlers/PlayerHandlers.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Handlers/TerrainHandlers.cpp

Loading...
View Options

source/tools/atlas/GameInterface/InputProcessor.cpp

Loading...
View Options

source/tools/atlas/GameInterface/Misc.cpp

Loading...
View Options

source/tools/atlas/GameInterface/SimState.cpp

Loading...
View Options

source/tools/atlas/GameInterface/View.cpp

Loading...
Wildfire Games · Phabricator