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

@@ -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<T, Tnum_files, Tsearch_in_tars>::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<T, Tnum_files, Tsearch_in_tars>::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<T, Tnum_files, Tsearch_in_tars>::file_names[i], false);
+		item = files->GetItem(BaseSet<T, Tnum_files, Tsearch_in_tars>::file_names[i]);
 		if (item == nullptr || (!item->value.has_value() && !allow_empty_filename)) {
 			Debug(grf, 0, "No " SET_TYPE " file for: {} (in {})", BaseSet<T, Tnum_files, Tsearch_in_tars>::file_names[i], full_filename);
 			return false;
@@ -93,7 +93,7 @@ bool BaseSet<T, Tnum_files, Tsearch_in_tars>::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<T, Tnum_files, Tsearch_in_tars>::FillSetDetails(IniFile *ini, const
 		}
 
 		/* Then find the warning message when the file's missing */
-		item = origin->GetItem(filename, false);
+		item = origin->GetItem(filename);
-		if (item == nullptr) item = origin->GetItem("default", false);
+		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();

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;

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);
+		IniItem &item = group->GetOrCreateItem(hotkey.name);
-		item->SetValue(SaveKeycodes(hotkey));
+		item.SetValue(SaveKeycodes(hotkey));
 	}
 }
 

src/ini_load.cpp

@@ -82,22 +82,32 @@ IniGroup::~IniGroup()
 }
 
 /**
- * Get the item with the given name, and if it doesn't exist
+ * Get the item with the given name.
- * and create is true it creates a new item.
  * @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));
 }
 
 /**

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();
 };

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) {

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->GetOrCreateItem("version_string").SetValue(_openttd_revision);
-	group->GetItem("version_number", true)->SetValue(fmt::format("{:08X}", _openttd_newgrf_version));
+	group->GetOrCreateItem("version_number").SetValue(fmt::format("{:08X}", _openttd_newgrf_version));
-	group->GetItem("ini_version", true)->SetValue(std::to_string(INIFILE_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;

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);
+	IniItem *item = grp->GetItem(name);
-	if (item == nullptr && defaults != nullptr) item = defaults->GetItem(name, false);
+	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);
 			}