Page MenuHomeWildfire Games

Remove references to globals
Needs ReviewPublic

Authored by Angen on Nov 9 2018, 7:31 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
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 6441
Build 10670: Vulcan BuildJenkins
Build 10669: arc lint + arc unit

Event Timeline

Angen 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/

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–89

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

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

Renaming did not applied another files

Angen 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/

smiley added a subscriber: smiley.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.

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

Even more redundant since there is a check in DeInitGame

Angen marked 3 inline comments as done.Nov 10 2018, 9:40 AM
Angen 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–89

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?

smiley 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.
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–89

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.

Angen marked 4 inline comments as done.Nov 10 2018, 11:00 AM
Angen 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

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

Angen 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.

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().

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

ok I missunderstood put the private under public