Changeset View
Standalone View
source/ps/ConfigDB.cpp
Show All 26 Lines | |||||
#include "ps/CStr.h" | #include "ps/CStr.h" | ||||
#include "ps/Filesystem.h" | #include "ps/Filesystem.h" | ||||
#include <mutex> | #include <mutex> | ||||
#include <unordered_set> | #include <unordered_set> | ||||
typedef std::map<CStr, CConfigValueSet> TConfigMap; | typedef std::map<CStr, CConfigValueSet> TConfigMap; | ||||
TConfigMap CConfigDB::m_Map[CFG_LAST]; | TConfigMap CConfigDB::m_Map[CFG_LAST]; | ||||
vladislavbelov: It's strange to has static members in singleton. | |||||
VfsPath CConfigDB::m_ConfigFile[CFG_LAST]; | VfsPath CConfigDB::m_ConfigFile[CFG_LAST]; | ||||
bool CConfigDB::m_HasChanges[CFG_LAST]; | bool CConfigDB::m_HasChanges[CFG_LAST]; | ||||
std::multimap<CStr, std::function<void()>> CConfigDB::m_Hooks; | |||||
Not Done Inline ActionsType duplication. vladislavbelov: Type duplication. | |||||
static std::recursive_mutex cfgdb_mutex; | static std::recursive_mutex cfgdb_mutex; | ||||
// These entries will not be printed to logfiles, so that logfiles can be shared without leaking personal or sensitive data | // These entries will not be printed to logfiles, so that logfiles can be shared without leaking personal or sensitive data | ||||
static const std::unordered_set<std::string> g_UnloggedEntries = { | static const std::unordered_set<std::string> g_UnloggedEntries = { | ||||
"lobby.password", | "lobby.password", | ||||
"lobby.buddies", | "lobby.buddies", | ||||
"userreport.id" // authentication token for GDPR personal data requests | "userreport.id" // authentication token for GDPR personal data requests | ||||
}; | }; | ||||
▲ Show 20 Lines • Show All 150 Lines • ▼ Show 20 Lines | void CConfigDB::SetValueString(EConfigNamespace ns, const CStr& name, const CStr& value) | ||||
CHECK_NS(;); | CHECK_NS(;); | ||||
std::lock_guard<std::recursive_mutex> s(cfgdb_mutex); | std::lock_guard<std::recursive_mutex> s(cfgdb_mutex); | ||||
TConfigMap::iterator it = m_Map[ns].find(name); | TConfigMap::iterator it = m_Map[ns].find(name); | ||||
if (it == m_Map[ns].end()) | if (it == m_Map[ns].end()) | ||||
it = m_Map[ns].insert(m_Map[ns].begin(), make_pair(name, CConfigValueSet(1))); | it = m_Map[ns].insert(m_Map[ns].begin(), make_pair(name, CConfigValueSet(1))); | ||||
it->second[0] = value; | it->second[0] = value; | ||||
std::for_each(m_Hooks.lower_bound(name), m_Hooks.upper_bound(name), [](std::pair<const CStr, std::function<void()>>& hook) { hook.second(); }); | |||||
Not Done Inline ActionsWhy types are different? const CStr vs CStr? vladislavbelov: Why types are different? `const CStr` vs `CStr`? | |||||
Done Inline ActionsDoesn't compile otherwise, the value_type of std::MultiMap is <const First Arg> (otherwise you could change the key) wraitii: Doesn't compile otherwise, the value_type of std::MultiMap is <const First Arg> (otherwise you… | |||||
Not Done Inline ActionsIn that definition - yes, but you just may (if you want) use const std::pair<CStr, std::function<void()>>& hook as you don't need to change the map element. vladislavbelov: In that definition - yes, but you just may (if you want) use `const std::pair<CStr, std… | |||||
} | } | ||||
void CConfigDB::SetValueBool(EConfigNamespace ns, const CStr& name, const bool value) | void CConfigDB::SetValueBool(EConfigNamespace ns, const CStr& name, const bool value) | ||||
{ | { | ||||
CStr valueString = value ? "true" : "false"; | CStr valueString = value ? "true" : "false"; | ||||
SetValueString(ns, name, valueString); | SetValueString(ns, name, valueString); | ||||
} | } | ||||
void CConfigDB::RemoveValue(EConfigNamespace ns, const CStr& name) | void CConfigDB::RemoveValue(EConfigNamespace ns, const CStr& name) | ||||
{ | { | ||||
CHECK_NS(;); | CHECK_NS(;); | ||||
std::lock_guard<std::recursive_mutex> s(cfgdb_mutex); | std::lock_guard<std::recursive_mutex> s(cfgdb_mutex); | ||||
TConfigMap::iterator it = m_Map[ns].find(name); | TConfigMap::iterator it = m_Map[ns].find(name); | ||||
if (it == m_Map[ns].end()) | if (it == m_Map[ns].end()) | ||||
return; | return; | ||||
m_Map[ns].erase(it); | m_Map[ns].erase(it); | ||||
std::for_each(m_Hooks.lower_bound(name), m_Hooks.upper_bound(name), [](std::pair<const CStr, std::function<void()>>& hook) { hook.second(); }); | |||||
Not Done Inline ActionsDuplication, should be in one function (like NotifyHooks). vladislavbelov: Duplication, should be in one function (like `NotifyHooks`). | |||||
} | } | ||||
void CConfigDB::SetConfigFile(EConfigNamespace ns, const VfsPath& path) | void CConfigDB::SetConfigFile(EConfigNamespace ns, const VfsPath& path) | ||||
{ | { | ||||
CHECK_NS(;); | CHECK_NS(;); | ||||
std::lock_guard<std::recursive_mutex> s(cfgdb_mutex); | std::lock_guard<std::recursive_mutex> s(cfgdb_mutex); | ||||
m_ConfigFile[ns] = path; | m_ConfigFile[ns] = path; | ||||
▲ Show 20 Lines • Show All 217 Lines • ▼ Show 20 Lines | bool CConfigDB::WriteValueToFile(EConfigNamespace ns, const CStr& name, const CStr& value, const VfsPath& path) | ||||
Reload(ns); | Reload(ns); | ||||
SetValueString(ns, name, value); | SetValueString(ns, name, value); | ||||
bool ret = WriteFile(ns, path); | bool ret = WriteFile(ns, path); | ||||
m_Map[ns].swap(newMap); | m_Map[ns].swap(newMap); | ||||
return ret; | return ret; | ||||
} | } | ||||
void CConfigDB::RegisterHook(const CStr& name, std::function<void()> hook) | |||||
{ | |||||
m_Hooks.insert({name, hook}); | |||||
Not Done Inline ActionsI'd prefer m_Hooks.emplace(name, std::move(hook)); if possible. vladislavbelov: I'd prefer `m_Hooks.emplace(name, std::move(hook));` if possible. | |||||
Done Inline ActionsStill not std::move. vladislavbelov: Still not `std::move`. | |||||
hook(); | |||||
} | |||||
#undef CHECK_NS | #undef CHECK_NS | ||||
Not Done Inline ActionsIs it guaranteed that hook exists in m_Hooks? vladislavbelov: Is it guaranteed that `hook` exists in `m_Hooks`? | |||||
Done Inline ActionsIt's not guaranteed by the compiler but I've changed hook_t to be an actual opaque type that can only be moved (outside of CConfigDB), so I don't think there's a way to generate a hook that isn't somewhere in m_Hooks (or at least not without doing something so obviously dangerous nobody would try/it'd be seen in reviews). wraitii: It's not guaranteed by the compiler but I've changed hook_t to be an actual opaque type that… | |||||
Done Inline ActionsThere could still have been a case where hook was deleted twice, so I changed UnregisterHook to accept only moved-from hooks, which ought to clear that case up. wraitii: There could still have been a case where hook was deleted twice, so I changed UnregisterHook to… | |||||
Not Done Inline ActionsThat's incorrect, iterator might be not equal to the end() after its deleting. vladislavbelov: That's incorrect, iterator might be not equal to the `end()` after its deleting. | |||||
Done Inline Actionsmoved-from iterators point to end (see above), so I don't see a path to generate an invalid-iterator-not-pointing-to-end, at least from outside this class? wraitii: moved-from iterators point to end (see above), so I don't see a path to generate an invalid… | |||||
Done Inline ActionsAs noted by Vlad on IRC, we might run into the static initialization order fiasco here, but we actually don't since CConfigDB is initialised at runtime. wraitii: As noted by Vlad on IRC, we might run into the static initialization order fiasco here, but we… |
It's strange to has static members in singleton.