From ca1c0007b63ebc1ca435d136797444f9aee15e01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guilloux?= Date: Thu, 18 Dec 2025 22:34:47 +0100 Subject: [PATCH] Codechange: [Script] Replace map of sets with set of pairs for per value storage (#14930) --- src/script/api/script_list.cpp | 130 ++++++++++----------------------- src/script/api/script_list.hpp | 7 +- 2 files changed, 43 insertions(+), 94 deletions(-) diff --git a/src/script/api/script_list.cpp b/src/script/api/script_list.cpp index ec7c58e4a6..a339967f10 100644 --- a/src/script/api/script_list.cpp +++ b/src/script/api/script_list.cpp @@ -50,7 +50,7 @@ public: */ bool IsEnd() { - return this->list->buckets.empty() || this->has_no_more_items; + return this->list->items.empty() || this->has_no_more_items; } /** @@ -75,9 +75,7 @@ public: */ class ScriptListSorterValueAscending : public ScriptListSorter { private: - ScriptList::ScriptListBucket::iterator bucket_iter; ///< The iterator over the list to find the buckets. - ScriptList::ScriptItemList *bucket_list; ///< The current bucket list we're iterator over. - ScriptList::ScriptItemList::iterator bucket_list_iter; ///< The iterator over the bucket list. + ScriptList::ScriptListSet::iterator value_iter; ///< The iterator over the value/item pairs in the set. public: /** @@ -92,16 +90,14 @@ public: std::optional Begin() override { - if (this->list->buckets.empty()) { + if (this->list->values.empty()) { this->item_next = std::nullopt; return std::nullopt; } this->has_no_more_items = false; - this->bucket_iter = this->list->buckets.begin(); - this->bucket_list = &this->bucket_iter->second; - this->bucket_list_iter = this->bucket_list->begin(); - this->item_next = *this->bucket_list_iter; + this->value_iter = this->list->values.begin(); + this->item_next = this->value_iter->second; std::optional item_current = this->item_next; this->FindNext(); @@ -110,7 +106,6 @@ public: void End() override { - this->bucket_list = nullptr; this->item_next = std::nullopt; this->has_no_more_items = true; } @@ -120,24 +115,13 @@ public: */ void FindNext() { - if (this->bucket_list == nullptr) { - this->item_next = std::nullopt; + this->item_next = std::nullopt; + if (this->value_iter == this->list->values.end()) { this->has_no_more_items = true; return; } - - ++this->bucket_list_iter; - if (this->bucket_list_iter == this->bucket_list->end()) { - ++this->bucket_iter; - if (this->bucket_iter == this->list->buckets.end()) { - this->bucket_list = nullptr; - this->item_next = std::nullopt; - return; - } - this->bucket_list = &this->bucket_iter->second; - this->bucket_list_iter = this->bucket_list->begin(); - } - this->item_next = *this->bucket_list_iter; + ++this->value_iter; + if (this->value_iter != this->list->values.end()) this->item_next = this->value_iter->second; } std::optional Next() override @@ -169,9 +153,7 @@ private: /* Note: We cannot use reverse_iterator. * The iterators must only be invalidated when the element they are pointing to is removed. * This only holds for forward iterators. */ - ScriptList::ScriptListBucket::iterator bucket_iter; ///< The iterator over the list to find the buckets. - ScriptList::ScriptItemList *bucket_list; ///< The current bucket list we're iterator over. - ScriptList::ScriptItemList::iterator bucket_list_iter; ///< The iterator over the bucket list. + ScriptList::ScriptListSet::iterator value_iter; ///< The iterator over the value/item pairs in the set. public: /** @@ -186,21 +168,15 @@ public: std::optional Begin() override { - if (this->list->buckets.empty()) { + if (this->list->values.empty()) { this->item_next = std::nullopt; return std::nullopt; } this->has_no_more_items = false; - /* Go to the end of the bucket-list */ - this->bucket_iter = this->list->buckets.end(); - --this->bucket_iter; - this->bucket_list = &this->bucket_iter->second; - - /* Go to the end of the items in the bucket */ - this->bucket_list_iter = this->bucket_list->end(); - --this->bucket_list_iter; - this->item_next = *this->bucket_list_iter; + this->value_iter = this->list->values.end(); + --this->value_iter; + this->item_next = this->value_iter->second; std::optional item_current = this->item_next; this->FindNext(); @@ -209,7 +185,6 @@ public: void End() override { - this->bucket_list = nullptr; this->item_next = std::nullopt; this->has_no_more_items = true; } @@ -219,25 +194,18 @@ public: */ void FindNext() { - if (this->bucket_list == nullptr) { - this->item_next = std::nullopt; + this->item_next = std::nullopt; + if (this->value_iter == this->list->values.end()) { this->has_no_more_items = true; return; } - - if (this->bucket_list_iter == this->bucket_list->begin()) { - if (this->bucket_iter == this->list->buckets.begin()) { - this->bucket_list = nullptr; - this->item_next = std::nullopt; - return; - } - --this->bucket_iter; - this->bucket_list = &this->bucket_iter->second; - /* Go to the end of the items in the bucket */ - this->bucket_list_iter = this->bucket_list->end(); + if (this->value_iter == this->list->values.begin()) { + /* Use 'end' as marker for 'beyond begin' */ + this->value_iter = this->list->values.end(); + } else { + --this->value_iter; } - --this->bucket_list_iter; - this->item_next = *this->bucket_list_iter; + if (this->value_iter != this->list->values.end()) this->item_next = this->value_iter->second; } std::optional Next() override @@ -483,7 +451,7 @@ void ScriptList::CopyList(const ScriptList *list) { this->Sort(list->sorter_type, list->sort_ascending); this->items = list->items; - this->buckets = list->buckets; + this->values = list->values; } ScriptList::ScriptList() @@ -510,7 +478,7 @@ void ScriptList::Clear() this->modifications++; this->items.clear(); - this->buckets.clear(); + this->values.clear(); this->sorter->End(); } @@ -521,7 +489,7 @@ void ScriptList::AddItem(SQInteger item, SQInteger value) if (this->HasItem(item)) return; this->items[item] = value; - this->buckets[value].insert(item); + this->values.emplace(value, item); } void ScriptList::RemoveItem(SQInteger item) @@ -534,10 +502,9 @@ void ScriptList::RemoveItem(SQInteger item) SQInteger value = item_iter->second; this->sorter->Remove(item); - auto bucket_iter = this->buckets.find(value); - assert(bucket_iter != this->buckets.end()); - bucket_iter->second.erase(item); - if (bucket_iter->second.empty()) this->buckets.erase(bucket_iter); + auto value_iter = this->values.find({value, item}); + assert(value_iter != this->values.end()); + this->values.erase(value_iter); this->items.erase(item_iter); } @@ -592,12 +559,11 @@ bool ScriptList::SetValue(SQInteger item, SQInteger value) if (value_old == value) return true; this->sorter->Remove(item); - auto bucket_iter = this->buckets.find(value_old); - assert(bucket_iter != this->buckets.end()); - bucket_iter->second.erase(item); - if (bucket_iter->second.empty()) this->buckets.erase(bucket_iter); + auto value_iter = this->values.find({value_old, item}); + assert(value_iter != this->values.end()); + this->values.erase(value_iter); item_iter->second = value; - this->buckets[value].insert(item); + this->values.emplace(value, item); return true; } @@ -640,7 +606,7 @@ void ScriptList::AddList(ScriptList *list) if (this->IsEmpty()) { /* If this is empty, we can just take the items of the other list as is. */ this->items = list->items; - this->buckets = list->buckets; + this->values = list->values; this->modifications++; } else { for (const auto &item : list->items) { @@ -655,7 +621,7 @@ void ScriptList::SwapList(ScriptList *list) if (list == this) return; this->items.swap(list->items); - this->buckets.swap(list->buckets); + this->values.swap(list->values); std::swap(this->sorter, list->sorter); std::swap(this->sorter_type, list->sorter_type); std::swap(this->sort_ascending, list->sort_ascending); @@ -699,17 +665,9 @@ void ScriptList::RemoveTop(SQInteger count) switch (this->sorter_type) { default: NOT_REACHED(); case SORT_BY_VALUE: - for (auto iter = this->buckets.begin(); iter != this->buckets.end(); iter = this->buckets.begin()) { - ScriptItemList *items = &iter->second; - size_t size = items->size(); - for (auto iter = items->begin(); iter != items->end(); iter = items->begin()) { - if (--count < 0) return; - this->RemoveItem(*iter); - /* When the last item is removed from the bucket, the bucket itself is removed. - * This means that the iterators can be invalid after a call to RemoveItem. - */ - if (--size == 0) break; - } + for (auto iter = this->values.begin(); iter != this->values.end(); iter = this->values.begin()) { + if (--count < 0) return; + this->RemoveItem(iter->second); } break; @@ -736,17 +694,9 @@ void ScriptList::RemoveBottom(SQInteger count) switch (this->sorter_type) { default: NOT_REACHED(); case SORT_BY_VALUE: - for (auto iter = this->buckets.rbegin(); iter != this->buckets.rend(); iter = this->buckets.rbegin()) { - ScriptItemList *items = &iter->second; - size_t size = items->size(); - for (auto iter = items->rbegin(); iter != items->rend(); iter = items->rbegin()) { - if (--count < 0) return; - this->RemoveItem(*iter); - /* When the last item is removed from the bucket, the bucket itself is removed. - * This means that the iterators can be invalid after a call to RemoveItem. - */ - if (--size == 0) break; - } + for (auto iter = this->values.rbegin(); iter != this->values.rend(); iter = this->values.rbegin()) { + if (--count < 0) return; + this->RemoveItem(iter->second); } break; diff --git a/src/script/api/script_list.hpp b/src/script/api/script_list.hpp index ef8b56ff0c..3e7e02b65e 100644 --- a/src/script/api/script_list.hpp +++ b/src/script/api/script_list.hpp @@ -166,12 +166,11 @@ protected: } public: - typedef std::set ScriptItemList; ///< The list of items inside the bucket - typedef std::map ScriptListBucket; ///< The bucket list per value - typedef std::map ScriptListMap; ///< List per item + using ScriptListSet = std::set>; ///< List per value + using ScriptListMap = std::map; ///< List per item ScriptListMap items; ///< The items in the list - ScriptListBucket buckets; ///< The items in the list, sorted by value + ScriptListSet values; ///< The items in the list, sorted by value ScriptList(); ~ScriptList();