From cdd555edd5ee3aca4811352658d614a544fdb10b Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Sun, 15 Jun 2025 21:32:29 +0100 Subject: [PATCH] Codechange: Use vector with unique_ptr instead of linked-list for base set lists. (#14332) --- src/base_media_base.h | 28 +++++----- src/base_media_func.h | 124 ++++++++++++++++-------------------------- src/gfxinit.cpp | 6 +- src/music.cpp | 4 +- src/sound.cpp | 4 +- 5 files changed, 69 insertions(+), 97 deletions(-) diff --git a/src/base_media_base.h b/src/base_media_base.h index b233ae0256..6286455b2d 100644 --- a/src/base_media_base.h +++ b/src/base_media_base.h @@ -70,14 +70,6 @@ struct BaseSet { uint found_files = 0; ///< Number of the files that could be found uint valid_files = 0; ///< Number of the files that could be found and are valid - T *next = nullptr; ///< The next base set in this list - - /** Free everything we allocated */ - ~BaseSet() - { - delete this->next; - } - /** * Get the number of missing files. * @return the number @@ -170,9 +162,9 @@ struct BaseSet { template class BaseMedia : FileScanner { protected: - static inline Tbase_set *available_sets = nullptr; ///< All available sets - static inline Tbase_set *duplicate_sets = nullptr; ///< All sets that aren't available, but needed for not downloading base sets when a newer version than the one on BaNaNaS is loaded. - static inline const Tbase_set *used_set = nullptr; ///< The currently used set + static inline std::vector> available_sets; ///< All available sets + static inline std::vector> duplicate_sets; ///< All sets that aren't available, but needed for not downloading base sets when a newer version than the one on BaNaNaS is loaded. + static inline const Tbase_set *used_set; ///< The currently used set bool AddFile(const std::string &filename, size_t basepath_length, const std::string &tar_filename) override; @@ -181,6 +173,12 @@ protected: * @return the extension */ static std::string_view GetExtension(); + + /** + * Return the duplicate sets. + * @return The duplicate sets. + */ + static std::span> GetDuplicateSets() { return BaseMedia::duplicate_sets; } public: /** * Determine the graphics pack that has to be used. @@ -198,7 +196,11 @@ public: return num + fs.Scan(GetExtension(), BASESET_DIR, Tbase_set::SEARCH_IN_TARS); } - static Tbase_set *GetAvailableSets(); + /** + * Return the available sets. + * @return The available sets. + */ + static std::span> GetAvailableSets() { return BaseMedia::available_sets; } static bool SetSet(const Tbase_set *set); static bool SetSetByName(const std::string &name); @@ -226,6 +228,6 @@ public: * @return The filename of the first file of the base set, or \c std::nullopt if there is no match. */ template -std::optional TryGetBaseSetFile(const ContentInfo &ci, bool md5sum, const Tbase_set *s); +std::optional TryGetBaseSetFile(const ContentInfo &ci, bool md5sum, std::span> sets); #endif /* BASE_MEDIA_BASE_H */ diff --git a/src/base_media_func.h b/src/base_media_func.h index aafd4e954a..8f1fac4c26 100644 --- a/src/base_media_func.h +++ b/src/base_media_func.h @@ -187,10 +187,9 @@ bool BaseSet::FillSetDetails(const IniFile &ini, const std::string &path, con template bool BaseMedia::AddFile(const std::string &filename, size_t basepath_length, const std::string &) { - bool ret = false; Debug(misc, 1, "Checking {} for base {} set", filename, BaseSet::SET_TYPE); - Tbase_set *set = new Tbase_set(); + auto set = std::make_unique(); IniFile ini{}; std::string path{ filename, basepath_length }; ini.LoadFromDisk(path, BASESET_DIR); @@ -202,60 +201,44 @@ bool BaseMedia::AddFile(const std::string &filename, size_t basepath_ path.clear(); } - if (set->FillSetDetails(ini, path, filename)) { - Tbase_set *duplicate = nullptr; - for (Tbase_set *c = BaseMedia::available_sets; c != nullptr; c = c->next) { - if (c->name == set->name || c->shortname == set->shortname) { - duplicate = c; - break; - } + if (!set->FillSetDetails(ini, path, filename)) return false; + + auto existing = std::ranges::find_if(BaseMedia::available_sets, [&set](const auto &c) { return c->name == set->name || c->shortname == set->shortname; }); + if (existing != std::end(BaseMedia::available_sets)) { + /* The more complete set takes precedence over the version number. */ + if (((*existing)->valid_files == set->valid_files && (*existing)->version >= set->version) || + (*existing)->valid_files > set->valid_files) { + + Debug(misc, 1, "Not adding {} ({}) as base {} set (duplicate, {})", set->name, fmt::join(set->version, "."), + BaseSet::SET_TYPE, + (*existing)->valid_files > set->valid_files ? "fewer valid files" : "lower version"); + + duplicate_sets.push_back(std::move(set)); + return false; } - if (duplicate != nullptr) { - /* The more complete set takes precedence over the version number. */ - if ((duplicate->valid_files == set->valid_files && duplicate->version >= set->version) || - duplicate->valid_files > set->valid_files) { - Debug(misc, 1, "Not adding {} ({}) as base {} set (duplicate, {})", set->name, fmt::join(set->version, "."), - BaseSet::SET_TYPE, - duplicate->valid_files > set->valid_files ? "less valid files" : "lower version"); - set->next = BaseMedia::duplicate_sets; - BaseMedia::duplicate_sets = set; - } else { - Tbase_set **prev = &BaseMedia::available_sets; - while (*prev != duplicate) prev = &(*prev)->next; - *prev = set; - set->next = duplicate->next; + /* If the duplicate set is currently used (due to rescanning this can happen) + * update the currently used set to the new one. This will 'lie' about the + * version number until a new game is started which isn't a big problem */ + if (BaseMedia::used_set == existing->get()) BaseMedia::used_set = set.get(); - /* Keep baseset configuration, if compatible */ - set->CopyCompatibleConfig(*duplicate); + /* Keep baseset configuration, if compatible */ + set->CopyCompatibleConfig(**existing); - /* If the duplicate set is currently used (due to rescanning this can happen) - * update the currently used set to the new one. This will 'lie' about the - * version number until a new game is started which isn't a big problem */ - if (BaseMedia::used_set == duplicate) BaseMedia::used_set = set; + Debug(misc, 1, "Removing {} ({}) as base {} set (duplicate, {})", (*existing)->name, fmt::join((*existing)->version, "."), BaseSet::SET_TYPE, + (*existing)->valid_files < set->valid_files ? "fewer valid files" : "lower version"); - Debug(misc, 1, "Removing {} ({}) as base {} set (duplicate, {})", duplicate->name, fmt::join(duplicate->version, "."), - BaseSet::SET_TYPE, - duplicate->valid_files < set->valid_files ? "less valid files" : "lower version"); - duplicate->next = BaseMedia::duplicate_sets; - BaseMedia::duplicate_sets = duplicate; - ret = true; - } - } else { - Tbase_set **last = &BaseMedia::available_sets; - while (*last != nullptr) last = &(*last)->next; + /* Existing set is worse, move it to duplicates and replace with the current set. */ + duplicate_sets.push_back(std::move(*existing)); - *last = set; - ret = true; - } - if (ret) { - Debug(misc, 1, "Adding {} ({}) as base {} set", set->name, fmt::join(set->version, "."), BaseSet::SET_TYPE); - } + Debug(misc, 1, "Adding {} ({}) as base {} set", set->name, fmt::join(set->version, "."), BaseSet::SET_TYPE); + *existing = std::move(set); } else { - delete set; + Debug(grf, 1, "Adding {} ({}) as base {} set", set->name, set->version, BaseSet::SET_TYPE); + available_sets.push_back(std::move(set)); } - return ret; + return true; } /** @@ -287,9 +270,9 @@ template return SetSet(nullptr); } - for (const Tbase_set *s = BaseMedia::available_sets; s != nullptr; s = s->next) { + for (const auto &s : BaseMedia::available_sets) { if (name == s->name) { - return SetSet(s); + return SetSet(s.get()); } } return false; @@ -307,9 +290,9 @@ template return SetSet(nullptr); } - for (const Tbase_set *s = BaseMedia::available_sets; s != nullptr; s = s->next) { + for (const auto &s : BaseMedia::available_sets) { if (shortname == s->shortname) { - return SetSet(s); + return SetSet(s.get()); } } return false; @@ -323,7 +306,7 @@ template /* static */ void BaseMedia::GetSetsList(std::back_insert_iterator &output_iterator) { fmt::format_to(output_iterator, "List of {} sets:\n", BaseSet::SET_TYPE); - for (const Tbase_set *s = BaseMedia::available_sets; s != nullptr; s = s->next) { + for (const auto &s : BaseMedia::available_sets) { fmt::format_to(output_iterator, "{:>18}: {}", s->name, s->GetDescription({})); int invalid = s->GetNumInvalid(); if (invalid != 0) { @@ -342,9 +325,9 @@ template #include "network/core/tcp_content_type.h" -template std::optional TryGetBaseSetFile(const ContentInfo &ci, bool md5sum, const Tbase_set *s) +template std::optional TryGetBaseSetFile(const ContentInfo &ci, bool md5sum, std::span> sets) { - for (; s != nullptr; s = s->next) { + for (const auto &s : sets) { if (s->GetNumMissing() != 0) continue; if (s->shortname != ci.unique_id) continue; @@ -362,8 +345,8 @@ template std::optional TryGetBaseSetFile(con template /* static */ bool BaseMedia::HasSet(const ContentInfo &ci, bool md5sum) { - return TryGetBaseSetFile(ci, md5sum, BaseMedia::available_sets).has_value() || - TryGetBaseSetFile(ci, md5sum, BaseMedia::duplicate_sets).has_value(); + return TryGetBaseSetFile(ci, md5sum, BaseMedia::GetAvailableSets()).has_value() || + TryGetBaseSetFile(ci, md5sum, BaseMedia::GetDuplicateSets()).has_value(); } /** @@ -373,12 +356,9 @@ template template /* static */ int BaseMedia::GetNumSets() { - int n = 0; - for (const Tbase_set *s = BaseMedia::available_sets; s != nullptr; s = s->next) { - if (s != BaseMedia::used_set && s->GetNumMissing() != 0) continue; - n++; - } - return n; + return std::ranges::count_if(BaseMedia::GetAvailableSets(), [](const auto &set) { + return set.get() == BaseMedia::used_set || set->GetNumMissing() == 0; + }); } /** @@ -389,8 +369,8 @@ template /* static */ int BaseMedia::GetIndexOfUsedSet() { int n = 0; - for (const Tbase_set *s = BaseMedia::available_sets; s != nullptr; s = s->next) { - if (s == BaseMedia::used_set) return n; + for (const auto &s : BaseMedia::available_sets) { + if (s.get() == BaseMedia::used_set) return n; if (s->GetNumMissing() != 0) continue; n++; } @@ -404,9 +384,9 @@ template template /* static */ const Tbase_set *BaseMedia::GetSet(int index) { - for (const Tbase_set *s = BaseMedia::available_sets; s != nullptr; s = s->next) { - if (s != BaseMedia::used_set && s->GetNumMissing() != 0) continue; - if (index == 0) return s; + for (const auto &s : BaseMedia::available_sets) { + if (s.get() != BaseMedia::used_set && s->GetNumMissing() != 0) continue; + if (index == 0) return s.get(); index--; } FatalError("Base{}::GetSet(): index {} out of range", BaseSet::SET_TYPE, index); @@ -421,13 +401,3 @@ template { return BaseMedia::used_set; } - -/** - * Return the available sets. - * @return The available sets. - */ -template -/* static */ Tbase_set *BaseMedia::GetAvailableSets() -{ - return BaseMedia::available_sets; -} diff --git a/src/gfxinit.cpp b/src/gfxinit.cpp index b6aa3888bf..deaaf405b2 100644 --- a/src/gfxinit.cpp +++ b/src/gfxinit.cpp @@ -480,7 +480,7 @@ template <> const GraphicsSet *best = nullptr; - auto IsBetter = [&best] (const auto *current) { + auto IsBetter = [&best] (const GraphicsSet *current) { /* Nothing chosen yet. */ if (best == nullptr) return true; /* Not being a fallback is better. */ @@ -495,11 +495,11 @@ template <> return best->palette != PAL_DOS && current->palette == PAL_DOS; }; - for (const GraphicsSet *c = BaseMedia::available_sets; c != nullptr; c = c->next) { + for (const auto &c : BaseMedia::available_sets) { /* Skip unusable sets */ if (c->GetNumMissing() != 0) continue; - if (IsBetter(c)) best = c; + if (IsBetter(c.get())) best = c.get(); } BaseMedia::used_set = best; diff --git a/src/music.cpp b/src/music.cpp index b49c6db252..bca4a671e4 100644 --- a/src/music.cpp +++ b/src/music.cpp @@ -95,7 +95,7 @@ template <> if (BaseMedia::used_set != nullptr) return true; const MusicSet *best = nullptr; - for (const MusicSet *c = BaseMedia::available_sets; c != nullptr; c = c->next) { + for (const auto &c : BaseMedia::available_sets) { if (c->GetNumMissing() != 0) continue; if (best == nullptr || @@ -103,7 +103,7 @@ template <> best->valid_files < c->valid_files || (best->valid_files == c->valid_files && (best->shortname == c->shortname && best->version < c->version))) { - best = c; + best = c.get(); } } diff --git a/src/sound.cpp b/src/sound.cpp index a9bc44c0f3..c56ff5cd91 100644 --- a/src/sound.cpp +++ b/src/sound.cpp @@ -268,7 +268,7 @@ template <> if (BaseMedia::used_set != nullptr) return true; const SoundsSet *best = nullptr; - for (const SoundsSet *c = BaseMedia::available_sets; c != nullptr; c = c->next) { + for (const auto &c : BaseMedia::available_sets) { /* Skip unusable sets */ if (c->GetNumMissing() != 0) continue; @@ -277,7 +277,7 @@ template <> best->valid_files < c->valid_files || (best->valid_files == c->valid_files && (best->shortname == c->shortname && best->version < c->version))) { - best = c; + best = c.get(); } }