De-singletonify CConfigDB, make hooks RAII.

CConfigDB is no longer a singleton, and its members are no longer
static.
This makes it possible to have several instances of it, useful for
testing of if we ever want to have different config profiles.

Also updates hooks to delete themselves on destruction, to fit a more
natural RAII pattern. Long-lived objects must take care to destroy hooks
before ConfigDB is reset.

Differential Revision: https://code.wildfiregames.com/D3866
This was SVN commit r25326.

source/graphics/ObjectManager.cpp

@@ -51,7 +51,7 @@ CObjectManager::CObjectManager(CMeshManager& meshManager, CSkeletonAnimManager&
 {
 	RegisterFileReloadFunc(ReloadChangedFileCB, this);
 
-	m_QualityHook = std::make_unique<CConfigDBHook>(g_ConfigDB.RegisterHookAndCall("max_actor_quality", [this]() { this->ActorQualityChanged(); }));
+	m_QualityHook = std::make_unique<CConfigDBHook>(g_ConfigDB.RegisterHookAndCall("max_actor_quality", [this]() { ActorQualityChanged(); }));
 
 	if (!CXeromyces::AddValidator(g_VFS, "actor", "art/actors/actor.rng"))
 		LOGERROR("CObjectManager: failed to load actor grammar file 'art/actors/actor.rng'");
@@ -61,8 +61,6 @@ CObjectManager::~CObjectManager()
 {
 	UnloadObjects();
 
-	g_ConfigDB.UnregisterHook(std::move(m_QualityHook));
-
 	UnregisterFileReloadFunc(ReloadChangedFileCB, this);
 }
 

source/gui/tests/test_GuiManager.h

@@ -26,9 +26,11 @@
 #include "ps/Hotkey.h"
 #include "ps/XML/Xeromyces.h"
 
+#include <memory>
+
 class TestGuiManager : public CxxTest::TestSuite
 {
-	CConfigDB* configDB;
+	std::unique_ptr<CConfigDB> configDB;
 public:
 
 	void setUp()
@@ -37,7 +39,7 @@ public:
 		TS_ASSERT_OK(g_VFS->Mount(L"", DataDir() / "mods" / "_test.gui" / "", VFS_MOUNT_MUST_EXIST));
 		TS_ASSERT_OK(g_VFS->Mount(L"cache", DataDir() / "_testcache" / "", 0, VFS_MAX_PRIORITY));
 
-		configDB = new CConfigDB;
+		configDB = std::make_unique<CConfigDB>();
 
 		CXeromyces::Startup();
 
@@ -48,7 +50,7 @@ public:
 	{
 		delete g_GUI;
 		CXeromyces::Terminate();
-		delete configDB;
+		configDB.reset();
 		g_VFS.reset();
 		DeleteDirectory(DataDir()/"_testcache");
 	}
@@ -189,7 +191,6 @@ public:
 		ScriptInterface::FromJSVal(prq, js_hotkey_pressed_value, hotkey_pressed_value);
 		TS_ASSERT_EQUALS(hotkey_pressed_value, false);
 
-		configDB->RemoveValue(CFG_SYSTEM, test_hotkey_name);
 		UnloadHotkeys();
 	}
 };

source/ps/ConfigDB.cpp

@@ -37,8 +37,6 @@ void TriggerAllHooks(const std::multimap<CStr, std::function<void()>>& hooks, co
 	std::for_each(hooks.lower_bound(name), hooks.upper_bound(name), [](const std::pair<CStr, std::function<void()>>& hook) { hook.second(); });
 }
 
-std::recursive_mutex g_ConfigDBMutex;
-
 // These entries will not be printed to logfiles, so that logfiles can be shared without leaking personal or sensitive data
 const std::unordered_set<std::string> g_UnloggedEntries = {
 	"lobby.password",
@@ -89,17 +87,12 @@ std::string EscapeString(const CStr& str)
 } // anonymous namespace
 
 typedef std::map<CStr, CConfigValueSet> TConfigMap;
-TConfigMap CConfigDB::m_Map[CFG_LAST];
-VfsPath CConfigDB::m_ConfigFile[CFG_LAST];
-bool CConfigDB::m_HasChanges[CFG_LAST];
-
-std::multimap<CStr, std::function<void()>> CConfigDB::m_Hooks;
 
 #define GETVAL(type)\
 	void CConfigDB::GetValue(EConfigNamespace ns, const CStr& name, type& value)\
 	{\
 		CHECK_NS(;);\
-		std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);\
+		std::lock_guard<std::recursive_mutex> s(m_Mutex);\
 		TConfigMap::iterator it = m_Map[CFG_COMMAND].find(name);\
 		if (it != m_Map[CFG_COMMAND].end())\
 		{\
@@ -126,11 +119,41 @@ GETVAL(double)
 GETVAL(std::string)
 #undef GETVAL
 
+std::unique_ptr<CConfigDB> g_ConfigDBPtr;
+
+void CConfigDB::Initialise()
+{
+	g_ConfigDBPtr = std::make_unique<CConfigDB>();
+}
+
+void CConfigDB::Shutdown()
+{
+	g_ConfigDBPtr.reset();
+}
+
+bool CConfigDB::IsInitialised()
+{
+	return !!g_ConfigDBPtr;
+}
+
+CConfigDB* CConfigDB::Instance()
+{
+	return g_ConfigDBPtr.get();
+}
+
+CConfigDB::CConfigDB()
+{
+}
+
+CConfigDB::~CConfigDB()
+{
+}
+
 bool CConfigDB::HasChanges(EConfigNamespace ns) const
 {
 	CHECK_NS(false);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	return m_HasChanges[ns];
 }
 
@@ -138,7 +161,7 @@ void CConfigDB::SetChanges(EConfigNamespace ns, bool value)
 {
 	CHECK_NS(;);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	m_HasChanges[ns] = value;
 }
 
@@ -146,8 +169,8 @@ void CConfigDB::GetValues(EConfigNamespace ns, const CStr& name, CConfigValueSet
 {
 	CHECK_NS(;);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
-	TConfigMap::iterator it = m_Map[CFG_COMMAND].find(name);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
+	TConfigMap::const_iterator it = m_Map[CFG_COMMAND].find(name);
 	if (it != m_Map[CFG_COMMAND].end())
 	{
 		values = it->second;
@@ -169,8 +192,8 @@ EConfigNamespace CConfigDB::GetValueNamespace(EConfigNamespace ns, const CStr& n
 {
 	CHECK_NS(CFG_LAST);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
-	TConfigMap::iterator it = m_Map[CFG_COMMAND].find(name);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
+	TConfigMap::const_iterator it = m_Map[CFG_COMMAND].find(name);
 	if (it != m_Map[CFG_COMMAND].end())
 		return CFG_COMMAND;
 
@@ -186,7 +209,7 @@ EConfigNamespace CConfigDB::GetValueNamespace(EConfigNamespace ns, const CStr& n
 
 std::map<CStr, CConfigValueSet> CConfigDB::GetValuesWithPrefix(EConfigNamespace ns, const CStr& prefix) const
 {
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	std::map<CStr, CConfigValueSet> ret;
 
 	CHECK_NS(ret);
@@ -209,7 +232,7 @@ void CConfigDB::SetValueString(EConfigNamespace ns, const CStr& name, const CStr
 {
 	CHECK_NS(;);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	TConfigMap::iterator it = m_Map[ns].find(name);
 	if (it == m_Map[ns].end())
 		it = m_Map[ns].insert(m_Map[ns].begin(), make_pair(name, CConfigValueSet(1)));
@@ -232,7 +255,7 @@ void CConfigDB::SetValueList(EConfigNamespace ns, const CStr& name, std::vector<
 {
 	CHECK_NS(;);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	TConfigMap::iterator it = m_Map[ns].find(name);
 	if (it == m_Map[ns].end())
 		it = m_Map[ns].insert(m_Map[ns].begin(), make_pair(name, CConfigValueSet(1)));
@@ -244,7 +267,7 @@ void CConfigDB::RemoveValue(EConfigNamespace ns, const CStr& name)
 {
 	CHECK_NS(;);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	TConfigMap::iterator it = m_Map[ns].find(name);
 	if (it == m_Map[ns].end())
 		return;
@@ -257,7 +280,7 @@ void CConfigDB::SetConfigFile(EConfigNamespace ns, const VfsPath& path)
 {
 	CHECK_NS(;);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	m_ConfigFile[ns] = path;
 }
 
@@ -265,7 +288,7 @@ bool CConfigDB::Reload(EConfigNamespace ns)
 {
 	CHECK_NS(false);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 
 	shared_ptr<u8> buffer;
 	size_t buflen;
@@ -428,7 +451,7 @@ bool CConfigDB::WriteFile(EConfigNamespace ns) const
 {
 	CHECK_NS(false);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	return WriteFile(ns, m_ConfigFile[ns]);
 }
 
@@ -436,7 +459,7 @@ bool CConfigDB::WriteFile(EConfigNamespace ns, const VfsPath& path) const
 {
 	CHECK_NS(false);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	shared_ptr<u8> buf;
 	AllocateAligned(buf, 1*MiB, maxSectorSize);
 	char* pos = (char*)buf.get();
@@ -468,7 +491,7 @@ bool CConfigDB::WriteValueToFile(EConfigNamespace ns, const CStr& name, const CS
 {
 	CHECK_NS(false);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	return WriteValueToFile(ns, name, value, m_ConfigFile[ns]);
 }
 
@@ -476,7 +499,7 @@ bool CConfigDB::WriteValueToFile(EConfigNamespace ns, const CStr& name, const CS
 {
 	CHECK_NS(false);
 
-	std::lock_guard<std::recursive_mutex> s(g_ConfigDBMutex);
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 
 	TConfigMap newMap;
 	m_Map[ns].swap(newMap);
@@ -491,13 +514,18 @@ bool CConfigDB::WriteValueToFile(EConfigNamespace ns, const CStr& name, const CS
 CConfigDBHook CConfigDB::RegisterHookAndCall(const CStr& name, std::function<void()> hook)
 {
 	hook();
+	std::lock_guard<std::recursive_mutex> s(m_Mutex);
 	return CConfigDBHook(*this, m_Hooks.emplace(name, hook));
 }
 
 void CConfigDB::UnregisterHook(CConfigDBHook&& hook)
 {
 	if (hook.m_Ptr != m_Hooks.end())
+	{
+		std::lock_guard<std::recursive_mutex> s(m_Mutex);
 		m_Hooks.erase(hook.m_Ptr);
+		hook.m_Ptr = m_Hooks.end();
+	}
 }
 
 void CConfigDB::UnregisterHook(std::unique_ptr<CConfigDBHook> hook)

source/ps/ConfigDB.h

@@ -29,9 +29,11 @@
 
 #include "lib/file/vfs/vfs_path.h"
 #include "ps/CStr.h"
-#include "ps/Singleton.h"
 
+#include <array>
 #include <map>
+#include <memory>
+#include <mutex>
 #include <vector>
 
 /**
@@ -59,12 +61,22 @@ using CConfigValueSet = std::vector<CStr>;
 // Actually defined below - requires access to CConfigDB.
 class CConfigDBHook;
 
-#define g_ConfigDB CConfigDB::GetSingleton()
+#define g_ConfigDB (*CConfigDB::Instance())
 
-class CConfigDB : public Singleton<CConfigDB>
+class CConfigDB
 {
 	friend CConfigDBHook;
 public:
+	CConfigDB();
+	~CConfigDB();
+	CConfigDB(const CConfigDB&) = delete;
+	CConfigDB(CConfigDB&&) = delete;
+
+	static void Initialise();
+	static void Shutdown();
+	static bool IsInitialised();
+	static CConfigDB* Instance();
+
 	/**
 	 * Attempt to retrieve the value of a config variable with the given name;
 	 * will search CFG_COMMAND first, and then all namespaces from the specified
@@ -178,17 +190,22 @@ public:
 	 * Register a simple lambda that will be called anytime the value changes in any namespace
 	 * This is simple on purpose, the hook is responsible for checking if it should do something.
 	 * When RegisterHookAndCall is called, the hook is immediately triggered.
+	 * NB: CConfigDBHook will auto-unregister the hook when destroyed,
+	 * so you can use it to tie the lifetime of the hook to your object.
+	 * The hook will be deleted alongside ConfigDB anyways.
 	 */
-	CConfigDBHook RegisterHookAndCall(const CStr& name, std::function<void()> hook);
+	[[nodiscard]] CConfigDBHook RegisterHookAndCall(const CStr& name, std::function<void()> hook);
 
 	void UnregisterHook(CConfigDBHook&& hook);
 	void UnregisterHook(std::unique_ptr<CConfigDBHook> hook);
 
 private:
-	static std::map<CStr, CConfigValueSet> m_Map[];
-	static std::multimap<CStr, std::function<void()>> m_Hooks;
-	static VfsPath m_ConfigFile[];
-	static bool m_HasChanges[];
+	std::array<std::map<CStr, CConfigValueSet>, CFG_LAST> m_Map;
+	std::multimap<CStr, std::function<void()>> m_Hooks;
+	std::array<VfsPath, CFG_LAST> m_ConfigFile;
+	std::array<bool, CFG_LAST> m_HasChanges;
+
+	mutable std::recursive_mutex m_Mutex;
 };
 
 class CConfigDBHook
@@ -196,12 +213,23 @@ class CConfigDBHook
 	friend class CConfigDB;
 public:
 	CConfigDBHook() = delete;
+	CConfigDBHook(const CConfigDBHook&) = delete;
 	// Point the moved-from hook to end, which is checked for in UnregisterHook,
 	// to avoid a double-erase error.
-	CConfigDBHook(CConfigDBHook&& h) : m_ConfigDB(h.m_ConfigDB) { m_Ptr = std::move(h.m_Ptr); h.m_Ptr = m_ConfigDB.m_Hooks.end(); }
-	CConfigDBHook(const CConfigDBHook&) = delete;
+	CConfigDBHook(CConfigDBHook&& h) : m_ConfigDB(h.m_ConfigDB)
+	{
+		m_Ptr = std::move(h.m_Ptr);
+		h.m_Ptr = m_ConfigDB.m_Hooks.end();
+	}
+	// Unregisters the hook. Must be called before the original ConfigDB gets deleted.
+	~CConfigDBHook()
+	{
+		m_ConfigDB.UnregisterHook(std::move(*this));
+	}
 private:
-	CConfigDBHook(CConfigDB& cdb, std::multimap<CStr, std::function<void()>>::iterator p) : m_ConfigDB(cdb), m_Ptr(p) {};
+	CConfigDBHook(CConfigDB& cdb, std::multimap<CStr, std::function<void()>>::iterator p)
+		: m_ConfigDB(cdb), m_Ptr(p)
+	{};
 
 	std::multimap<CStr, std::function<void()>>::iterator m_Ptr;
 	CConfigDB& m_ConfigDB;

source/ps/GameSetup/Config.cpp

@@ -1,4 +1,4 @@
-/* Copyright (C) 2020 Wildfire Games.
+/* Copyright (C) 2021 Wildfire Games.
  * This file is part of 0 A.D.
  *
  * 0 A.D. is free software: you can redistribute it and/or modify
@@ -126,7 +126,7 @@ void CONFIG_Init(const CmdLineArgs& args)
 {
 	TIMER(L"CONFIG_Init");
 
-	new CConfigDB;
+	CConfigDB::Initialise();
 
 	// Load the global, default config file
 	g_ConfigDB.SetConfigFile(CFG_DEFAULT, L"config/default.cfg");

source/ps/GameSetup/GameSetup.cpp

@@ -719,7 +719,7 @@ void Shutdown(int flags)
 
 from_config:
 	TIMER_BEGIN(L"shutdown ConfigDB");
-	delete &g_ConfigDB;
+	CConfigDB::Shutdown();
 	TIMER_END(L"shutdown ConfigDB");
 
 	SAFE_DELETE(g_Console);

source/ps/Joystick.cpp

@@ -1,4 +1,4 @@
-/* Copyright (C) 2014 Wildfire Games.
+/* Copyright (C) 2021 Wildfire Games.
  * This file is part of 0 A.D.
  *
  * 0 A.D. is free software: you can redistribute it and/or modify
@@ -33,6 +33,8 @@ CJoystick::CJoystick() :
 void CJoystick::Initialise()
 {
 	bool joystickEnable = false;
+	if (!CConfigDB::IsInitialised())
+		return;
 	CFG_GET_VAL("joystick.enable", joystickEnable);
 	if (!joystickEnable)
 		return;

source/ps/tests/test_ConfigDB.h

@@ -20,11 +20,13 @@
 #include "lib/file/vfs/vfs.h"
 #include "ps/ConfigDB.h"
 
+#include <memory>
+
 extern PIVFS g_VFS;
 
 class TestConfigDB : public CxxTest::TestSuite
 {
-	CConfigDB* configDB;
+	std::unique_ptr<CConfigDB> configDB;
 public:
 
 	void setUp()
@@ -32,15 +34,14 @@ public:
 		g_VFS = CreateVfs();
 		TS_ASSERT_OK(g_VFS->Mount(L"config", DataDir() / "_testconfig" / ""));
 
-		configDB = new CConfigDB;
+		configDB = std::make_unique<CConfigDB>();
 	}
 
 	void tearDown()
 	{
 		DeleteDirectory(DataDir()/"_testconfig");
 		g_VFS.reset();
-
-		delete configDB;
+		configDB.reset();
 	}
 
 	void test_setting_int()

source/ps/tests/test_Hotkeys.h

@@ -31,7 +31,7 @@
 
 class TestHotkey : public CxxTest::TestSuite
 {
-	CConfigDB* configDB;
+	std::unique_ptr<CConfigDB> configDB;
 	// Stores whether one of these was sent in the last fakeInput call.
 	bool hotkeyPress = false;
 	bool hotkeyUp = false;
@@ -64,14 +64,14 @@ public:
 		TS_ASSERT_OK(g_VFS->Mount(L"config", DataDir() / "_testconfig" / ""));
 		TS_ASSERT_OK(g_VFS->Mount(L"cache", DataDir() / "_testcache" / ""));
 
-		configDB = new CConfigDB;
+		configDB = std::make_unique<CConfigDB>();
 
 		g_scancodes = {};
 	}
 
 	void tearDown()
 	{
-		delete configDB;
+		configDB.reset();
 		g_VFS.reset();
 		DeleteDirectory(DataDir()/"_testcache");
 		DeleteDirectory(DataDir()/"_testconfig");

source/renderer/RenderingOptions.cpp

@@ -185,9 +185,6 @@ void CRenderingOptions::ReadConfigAndSetupHooks()
 
 void CRenderingOptions::ClearHooks()
 {
-	if (CConfigDB::IsInitialised())
-		for (CConfigDBHook& hook : *m_ConfigHooks)
-			g_ConfigDB.UnregisterHook(std::move(hook));
 	m_ConfigHooks->clear();
 }
 

source/renderer/RenderingOptions.h

@@ -27,6 +27,7 @@
 #ifndef INCLUDED_RENDERINGOPTIONS
 #define INCLUDED_RENDERINGOPTIONS
 
+class CConfigDB;
 class CStr8;
 class CRenderer;