From 64d6ad50f9360cdf7418fb46cc5560739fe4f648 Mon Sep 17 00:00:00 2001 From: PeterN Date: Mon, 5 Jun 2023 19:29:52 +0100 Subject: [PATCH] Codechange: Split GetItem with GetOrCreateItem. (#10952) `IniGroup::GetItem()` returns nullptr if the item does not exist, but does not if the create parameter is set to true. Resolve CodeQL warnings with `GetOrCreateItem()` which returns a reference to the item instead. --- src/base_media_func.h | 12 ++++++------ src/gfxinit.cpp | 2 +- src/hotkeys.cpp | 6 +++--- src/ini_load.cpp | 24 +++++++++++++++++------- src/ini_type.h | 3 ++- src/music.cpp | 6 +++--- src/settings.cpp | 31 +++++++++++++++---------------- src/settingsgen/settingsgen.cpp | 8 ++++---- 8 files changed, 51 insertions(+), 41 deletions(-) diff --git a/src/base_media_func.h b/src/base_media_func.h index b5ffe7dc8a..2ad0028375 100644 --- a/src/base_media_func.h +++ b/src/base_media_func.h @@ -23,7 +23,7 @@ extern void CheckExternalFiles(); * @param name the name of the item to fetch. */ #define fetch_metadata(name) \ - item = metadata->GetItem(name, false); \ + item = metadata->GetItem(name); \ if (item == nullptr || !item->value.has_value() || item->value->empty()) { \ Debug(grf, 0, "Base " SET_TYPE "set detail loading: {} field missing.", name); \ Debug(grf, 0, " Is {} readable for the user running OpenTTD?", full_filename); \ @@ -65,7 +65,7 @@ bool BaseSet::FillSetDetails(IniFile *ini, const fetch_metadata("version"); this->version = atoi(item->value->c_str()); - item = metadata->GetItem("fallback", false); + item = metadata->GetItem("fallback"); this->fallback = (item != nullptr && item->value && *item->value != "0" && *item->value != "false"); /* For each of the file types we want to find the file, MD5 checksums and warning messages. */ @@ -75,7 +75,7 @@ bool BaseSet::FillSetDetails(IniFile *ini, const for (uint i = 0; i < Tnum_files; i++) { MD5File *file = &this->files[i]; /* Find the filename first. */ - item = files->GetItem(BaseSet::file_names[i], false); + item = files->GetItem(BaseSet::file_names[i]); if (item == nullptr || (!item->value.has_value() && !allow_empty_filename)) { Debug(grf, 0, "No " SET_TYPE " file for: {} (in {})", BaseSet::file_names[i], full_filename); return false; @@ -93,7 +93,7 @@ bool BaseSet::FillSetDetails(IniFile *ini, const file->filename = path + filename; /* Then find the MD5 checksum */ - item = md5s->GetItem(filename, false); + item = md5s->GetItem(filename); if (item == nullptr || !item->value.has_value()) { Debug(grf, 0, "No MD5 checksum specified for: {} (in {})", filename, full_filename); return false; @@ -119,8 +119,8 @@ bool BaseSet::FillSetDetails(IniFile *ini, const } /* Then find the warning message when the file's missing */ - item = origin->GetItem(filename, false); - if (item == nullptr) item = origin->GetItem("default", false); + item = origin->GetItem(filename); + if (item == nullptr) item = origin->GetItem("default"); if (item == nullptr || !item->value.has_value()) { Debug(grf, 1, "No origin warning message specified for: {}", filename); file->missing_warning.clear(); diff --git a/src/gfxinit.cpp b/src/gfxinit.cpp index b14579ccb3..3ebead6daa 100644 --- a/src/gfxinit.cpp +++ b/src/gfxinit.cpp @@ -357,7 +357,7 @@ bool GraphicsSet::FillSetDetails(IniFile *ini, const std::string &path, const st this->palette = ((*item->value)[0] == 'D' || (*item->value)[0] == 'd') ? PAL_DOS : PAL_WINDOWS; /* Get optional blitter information. */ - item = metadata->GetItem("blitter", false); + item = metadata->GetItem("blitter"); this->blitter = (item != nullptr && (*item->value)[0] == '3') ? BLT_32BPP : BLT_8BPP; } return ret; diff --git a/src/hotkeys.cpp b/src/hotkeys.cpp index 3cb9d35299..0739238db8 100644 --- a/src/hotkeys.cpp +++ b/src/hotkeys.cpp @@ -278,7 +278,7 @@ void HotkeyList::Load(IniFile *ini) { IniGroup *group = ini->GetGroup(this->ini_group); for (Hotkey &hotkey : this->items) { - IniItem *item = group->GetItem(hotkey.name, false); + IniItem *item = group->GetItem(hotkey.name); if (item != nullptr) { hotkey.keycodes.clear(); if (item->value.has_value()) ParseHotkeys(hotkey, item->value->c_str()); @@ -294,8 +294,8 @@ void HotkeyList::Save(IniFile *ini) const { IniGroup *group = ini->GetGroup(this->ini_group); for (const Hotkey &hotkey : this->items) { - IniItem *item = group->GetItem(hotkey.name, true); - item->SetValue(SaveKeycodes(hotkey)); + IniItem &item = group->GetOrCreateItem(hotkey.name); + item.SetValue(SaveKeycodes(hotkey)); } } diff --git a/src/ini_load.cpp b/src/ini_load.cpp index 06d6efb33c..3f209444ec 100644 --- a/src/ini_load.cpp +++ b/src/ini_load.cpp @@ -82,22 +82,32 @@ IniGroup::~IniGroup() } /** - * Get the item with the given name, and if it doesn't exist - * and create is true it creates a new item. + * Get the item with the given name. * @param name name of the item to find. - * @param create whether to create an item when not found or not. * @return the requested item or nullptr if not found. */ -IniItem *IniGroup::GetItem(const std::string &name, bool create) +IniItem *IniGroup::GetItem(const std::string &name) { for (IniItem *item = this->item; item != nullptr; item = item->next) { if (item->name == name) return item; } - if (!create) return nullptr; + return nullptr; +} - /* otherwise make a new one */ - return new IniItem(this, name); +/** + * Get the item with the given name, and if it doesn't exist create a new item. + * @param name name of the item to find. + * @return the requested item. + */ +IniItem &IniGroup::GetOrCreateItem(const std::string &name) +{ + for (IniItem *item = this->item; item != nullptr; item = item->next) { + if (item->name == name) return *item; + } + + /* Item doesn't exist, make a new one. */ + return *(new IniItem(this, name)); } /** diff --git a/src/ini_type.h b/src/ini_type.h index 0d50b58347..580125ff6a 100644 --- a/src/ini_type.h +++ b/src/ini_type.h @@ -44,7 +44,8 @@ struct IniGroup { IniGroup(struct IniLoadFile *parent, const std::string &name); ~IniGroup(); - IniItem *GetItem(const std::string &name, bool create); + IniItem *GetItem(const std::string &name); + IniItem &GetOrCreateItem(const std::string &name); void RemoveItem(const std::string &name); void Clear(); }; diff --git a/src/music.cpp b/src/music.cpp index 9c4392be4a..6c69d5e625 100644 --- a/src/music.cpp +++ b/src/music.cpp @@ -133,7 +133,7 @@ bool MusicSet::FillSetDetails(IniFile *ini, const std::string &path, const std:: this->songinfo[i].filename = filename; // non-owned pointer - IniItem *item = catindex->GetItem(_music_file_names[i], false); + IniItem *item = catindex->GetItem(_music_file_names[i]); if (item != nullptr && item->value.has_value() && !item->value->empty()) { /* Song has a CAT file index, assume it's MPS MIDI format */ this->songinfo[i].filetype = MTT_MPSMIDI; @@ -158,7 +158,7 @@ bool MusicSet::FillSetDetails(IniFile *ini, const std::string &path, const std:: * the beginning, so we don't start reading e.g. root. */ while (*trimmed_filename == PATHSEPCHAR) trimmed_filename++; - item = names->GetItem(trimmed_filename, false); + item = names->GetItem(trimmed_filename); if (item != nullptr && item->value.has_value() && !item->value->empty()) break; } @@ -179,7 +179,7 @@ bool MusicSet::FillSetDetails(IniFile *ini, const std::string &path, const std:: this->songinfo[i].tracknr = tracknr++; } - item = trimmed_filename != nullptr ? timingtrim->GetItem(trimmed_filename, false) : nullptr; + item = trimmed_filename != nullptr ? timingtrim->GetItem(trimmed_filename) : nullptr; if (item != nullptr && item->value.has_value() && !item->value->empty()) { auto endpos = item->value->find(':'); if (endpos != std::string::npos) { diff --git a/src/settings.cpp b/src/settings.cpp index 7ad980d172..6087957940 100644 --- a/src/settings.cpp +++ b/src/settings.cpp @@ -590,17 +590,17 @@ static void IniLoadSettings(IniFile &ini, const SettingTable &settings_table, co group = group_def; } - IniItem *item = group->GetItem(s, false); + IniItem *item = group->GetItem(s); if (item == nullptr && group != group_def) { /* For settings.xx.yy load the settings from [settings] yy = ? in case the previous * did not exist (e.g. loading old config files with a [settings] section */ - item = group_def->GetItem(s, false); + item = group_def->GetItem(s); } if (item == nullptr) { /* For settings.xx.zz.yy load the settings from [zz] yy = ? in case the previous * did not exist (e.g. loading old config files with a [yapf] section */ sc = s.find('.'); - if (sc != std::string::npos) item = ini.GetGroup(s.substr(0, sc))->GetItem(s.substr(sc + 1), false); + if (sc != std::string::npos) item = ini.GetGroup(s.substr(0, sc))->GetItem(s.substr(sc + 1)); } sd->ParseValue(item, object); @@ -649,7 +649,6 @@ void ListSettingDesc::ParseValue(const IniItem *item, void *object) const static void IniSaveSettings(IniFile &ini, const SettingTable &settings_table, const char *grpname, void *object, bool) { IniGroup *group_def = nullptr, *group; - IniItem *item; for (auto &desc : settings_table) { const SettingDesc *sd = GetSettingDesc(desc); @@ -669,11 +668,11 @@ static void IniSaveSettings(IniFile &ini, const SettingTable &settings_table, co group = group_def; } - item = group->GetItem(s, true); + IniItem &item = group->GetOrCreateItem(s); - if (!item->value.has_value() || !sd->IsSameValue(item, object)) { + if (!item.value.has_value() || !sd->IsSameValue(&item, object)) { /* The value is different, that means we have to write it to the ini */ - item->value.emplace(sd->FormatValue(object)); + item.value.emplace(sd->FormatValue(object)); } } } @@ -773,7 +772,7 @@ static void IniSaveSettingList(IniFile &ini, const char *grpname, StringList &li group->Clear(); for (const auto &iter : list) { - group->GetItem(iter, true)->SetValue(""); + group->GetOrCreateItem(iter).SetValue(""); } } @@ -1060,7 +1059,7 @@ static IniFileVersion LoadVersionFromConfig(IniFile &ini) { IniGroup *group = ini.GetGroup("version"); - auto version_number = group->GetItem("ini_version", false); + auto version_number = group->GetItem("ini_version"); /* Older ini-file versions don't have this key yet. */ if (version_number == nullptr || !version_number->value.has_value()) return IFV_0; @@ -1121,9 +1120,9 @@ static void GameSaveConfig(IniFile &ini, const char *grpname) static void SaveVersionInConfig(IniFile &ini) { IniGroup *group = ini.GetGroup("version"); - group->GetItem("version_string", true)->SetValue(_openttd_revision); - group->GetItem("version_number", true)->SetValue(fmt::format("{:08X}", _openttd_newgrf_version)); - group->GetItem("ini_version", true)->SetValue(std::to_string(INIFILE_VERSION)); + group->GetOrCreateItem("version_string").SetValue(_openttd_revision); + group->GetOrCreateItem("version_number").SetValue(fmt::format("{:08X}", _openttd_newgrf_version)); + group->GetOrCreateItem("ini_version").SetValue(std::to_string(INIFILE_VERSION)); } /* Save a GRF configuration to the given group name */ @@ -1136,7 +1135,7 @@ static void GRFSaveConfig(IniFile &ini, const char *grpname, const GRFConfig *li for (c = list; c != nullptr; c = c->next) { std::string key = fmt::format("{:08X}|{}|{}", BSWAP32(c->ident.grfid), FormatArrayAsHex(c->ident.md5sum), c->filename); - group->GetItem(key, true)->SetValue(GRFBuildParamList(c)); + group->GetOrCreateItem(key).SetValue(GRFBuildParamList(c)); } } @@ -1224,7 +1223,7 @@ void LoadFromConfig(bool startup) if (_settings_client.network.server_game_type == SERVER_GAME_TYPE_LOCAL) { IniGroup *network = generic_ini.GetGroup("network", false); if (network != nullptr) { - IniItem *server_advertise = network->GetItem("server_advertise", false); + IniItem *server_advertise = network->GetItem("server_advertise"); if (server_advertise != nullptr && server_advertise->value == "true") { _settings_client.network.server_game_type = SERVER_GAME_TYPE_PUBLIC; } @@ -1240,7 +1239,7 @@ void LoadFromConfig(bool startup) if (generic_version < IFV_NETWORK_PRIVATE_SETTINGS) { IniGroup *network = generic_ini.GetGroup("network", false); if (network != nullptr) { - IniItem *no_http_content_downloads = network->GetItem("no_http_content_downloads", false); + IniItem *no_http_content_downloads = network->GetItem("no_http_content_downloads"); if (no_http_content_downloads != nullptr) { if (no_http_content_downloads->value == "true") { _settings_client.network.no_http_content_downloads = true; @@ -1249,7 +1248,7 @@ void LoadFromConfig(bool startup) } } - IniItem *use_relay_service = network->GetItem("use_relay_service", false); + IniItem *use_relay_service = network->GetItem("use_relay_service"); if (use_relay_service != nullptr) { if (use_relay_service->value == "never") { _settings_client.network.use_relay_service = UseRelayService::URS_NEVER; diff --git a/src/settingsgen/settingsgen.cpp b/src/settingsgen/settingsgen.cpp index 396b2a66fc..a160205d22 100644 --- a/src/settingsgen/settingsgen.cpp +++ b/src/settingsgen/settingsgen.cpp @@ -229,8 +229,8 @@ static void DumpGroup(IniLoadFile *ifile, const char * const group_name) */ static const char *FindItemValue(const char *name, IniGroup *grp, IniGroup *defaults) { - IniItem *item = grp->GetItem(name, false); - if (item == nullptr && defaults != nullptr) item = defaults->GetItem(name, false); + IniItem *item = grp->GetItem(name); + if (item == nullptr && defaults != nullptr) item = defaults->GetItem(name); if (item == nullptr || !item->value.has_value()) return nullptr; return item->value->c_str(); } @@ -321,14 +321,14 @@ static void DumpSections(IniLoadFile *ifile) for (sgn = special_group_names; *sgn != nullptr; sgn++) if (grp->name == *sgn) break; if (*sgn != nullptr) continue; - IniItem *template_item = templates_grp->GetItem(grp->name, false); // Find template value. + IniItem *template_item = templates_grp->GetItem(grp->name); // Find template value. if (template_item == nullptr || !template_item->value.has_value()) { FatalError("Cannot find template {}", grp->name); } DumpLine(template_item, grp, default_grp, _stored_output); if (validation_grp != nullptr) { - IniItem *validation_item = validation_grp->GetItem(grp->name, false); // Find template value. + IniItem *validation_item = validation_grp->GetItem(grp->name); // Find template value. if (validation_item != nullptr && validation_item->value.has_value()) { DumpLine(validation_item, grp, default_grp, _post_amble_output); }