From 998b70d69826aca89af1218310f2c0c8b6a9588c Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sun, 7 Jun 2020 13:19:57 +0100 Subject: [PATCH] Make PluginMetadata::GetLoadAfterFiles() return a std::vector The collection is only ever iterated over, and the uniqueness and ordering don't matter. --- CMakeLists.txt | 1 + include/loot/metadata/plugin_metadata.h | 6 +- src/api/helpers/collections.h | 68 +++++++++++++++++++ src/api/metadata/condition_evaluator.cpp | 7 +- src/api/metadata/plugin_metadata.cpp | 37 ++++------ src/api/metadata/yaml/plugin_metadata.h | 2 +- src/api/sorting/plugin_sorting_data.cpp | 6 +- src/api/sorting/plugin_sorting_data.h | 8 +-- .../api/interface/database_interface_test.h | 14 ++-- .../metadata/condition_evaluator_test.h | 7 +- .../internals/metadata/plugin_metadata_test.h | 8 +-- src/tests/api/internals/metadata_list_test.h | 2 +- .../api/internals/sorting/plugin_sort_test.h | 2 +- 13 files changed, 113 insertions(+), 55 deletions(-) create mode 100644 src/api/helpers/collections.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 0e95377c..23169e46 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -272,6 +272,7 @@ set (LIBLOOT_HEADERS "${CMAKE_SOURCE_DIR}/include/loot/api.h" "${CMAKE_SOURCE_DIR}/src/api/sorting/plugin_graph.h" "${CMAKE_SOURCE_DIR}/src/api/sorting/plugin_sorting_data.h" "${CMAKE_SOURCE_DIR}/src/api/helpers/git_helper.h" + "${CMAKE_SOURCE_DIR}/src/api/helpers/collections.h" "${CMAKE_SOURCE_DIR}/src/api/helpers/crc.h" "${CMAKE_SOURCE_DIR}/src/api/helpers/logging.h" "${CMAKE_SOURCE_DIR}/src/api/helpers/text.h") diff --git a/include/loot/metadata/plugin_metadata.h b/include/loot/metadata/plugin_metadata.h index 04023e77..82200009 100644 --- a/include/loot/metadata/plugin_metadata.h +++ b/include/loot/metadata/plugin_metadata.h @@ -100,7 +100,7 @@ public: * Get the plugins that the plugin must load after. * @return The plugins that the plugin must load after. */ - LOOT_API std::set GetLoadAfterFiles() const; + LOOT_API std::vector GetLoadAfterFiles() const; /** * Get the files that the plugin requires to be installed. @@ -170,7 +170,7 @@ public: * @param after * The files to set. */ - LOOT_API void SetLoadAfterFiles(const std::set& after); + LOOT_API void SetLoadAfterFiles(const std::vector& after); /** * Set the files that the plugin requires to be installed. @@ -268,7 +268,7 @@ public: private: std::string name_; std::optional group_; - std::set loadAfter_; + std::vector loadAfter_; std::set requirements_; std::set incompatibilities_; std::vector messages_; diff --git a/src/api/helpers/collections.h b/src/api/helpers/collections.h new file mode 100644 index 00000000..9e5cafcc --- /dev/null +++ b/src/api/helpers/collections.h @@ -0,0 +1,68 @@ +/* LOOT + + A load order optimisation tool for Oblivion, Skyrim, Fallout 3 and + Fallout: New Vegas. + + Copyright (C) 2012-2016 WrinklyNinja + + This file is part of LOOT. + + LOOT is free software: you can redistribute + it and/or modify it under the terms of the GNU General Public License + as published by the Free Software Foundation, either version 3 of + the License, or (at your option) any later version. + + LOOT is distributed in the hope that it will + be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with LOOT. If not, see + . + */ + +#ifndef LOOT_API_HELPERS_COLLECTIONS +#define LOOT_API_HELPERS_COLLECTIONS + +#include +#include + +namespace loot { +// Append second to first, skipping any elements that are already present in +// first. Although this is O(U * M), both input vectors are expected to be +// small (with tens of elements being an unusually large number). +template +std::vector mergeVectors(std::vector first, + const std::vector& second) { + auto initialSizeOfFirst = first.size(); + for (const auto& element : second) { + auto end = first.cbegin() + initialSizeOfFirst; + + if (std::find(first.cbegin(), end, element) == end) { + first.push_back(element); + } + } + + return first; +} + +// Returns the elements in first that are not in second. Although this is +// O(F * S), both input vectors are expected to be small (with tens of elements +// being an unusually large number). +template +std::vector diffVectors(const std::vector& first, + const std::vector& second) { + std::vector result; + + for (const auto& element : first) { + if (std::find(second.begin(), second.end(), element) == second.end()) { + result.push_back(element); + } + } + + return result; +} +} + +#endif diff --git a/src/api/metadata/condition_evaluator.cpp b/src/api/metadata/condition_evaluator.cpp index 89177f94..fa88c328 100644 --- a/src/api/metadata/condition_evaluator.cpp +++ b/src/api/metadata/condition_evaluator.cpp @@ -127,13 +127,14 @@ PluginMetadata ConditionEvaluator::EvaluateAll(const PluginMetadata& pluginMetad evaluatedMetadata.SetGroup(pluginMetadata.GetGroup().value()); } - std::set fileSet; + std::vector files; for (const auto& file : pluginMetadata.GetLoadAfterFiles()) { if (Evaluate(file.GetCondition())) - fileSet.insert(file); + files.push_back(file); } - evaluatedMetadata.SetLoadAfterFiles(fileSet); + evaluatedMetadata.SetLoadAfterFiles(files); + std::set fileSet; fileSet.clear(); for (const auto& file : pluginMetadata.GetRequirements()) { if (Evaluate(file.GetCondition())) diff --git a/src/api/metadata/plugin_metadata.cpp b/src/api/metadata/plugin_metadata.cpp index 29eb11f7..f98f8fb6 100644 --- a/src/api/metadata/plugin_metadata.cpp +++ b/src/api/metadata/plugin_metadata.cpp @@ -30,6 +30,7 @@ #include #include "api/game/game.h" +#include "api/helpers/collections.h" #include "api/helpers/logging.h" #include "api/helpers/text.h" @@ -42,8 +43,7 @@ using std::vector; namespace loot { PluginMetadata::PluginMetadata() {} -PluginMetadata::PluginMetadata(const std::string& n) : - name_(n) { +PluginMetadata::PluginMetadata(const std::string& n) : name_(n) { // If the name passed ends in '.ghost', that should be trimmed. if (boost::iends_with(name_, ".ghost")) name_ = name_.substr(0, name_.length() - 6); @@ -57,12 +57,7 @@ void PluginMetadata::MergeMetadata(const PluginMetadata& plugin) { group_ = plugin.GetGroup(); } - // Merge the following. If any files in the source already exist in the - // destination, they will be skipped. Files have display strings and - // condition strings which aren't considered when comparing them, so - // will be lost if the plugin being merged in has additional data in - // these strings. - loadAfter_.insert(begin(plugin.loadAfter_), end(plugin.loadAfter_)); + loadAfter_ = mergeVectors(loadAfter_, plugin.loadAfter_); requirements_.insert(begin(plugin.requirements_), end(plugin.requirements_)); incompatibilities_.insert(begin(plugin.incompatibilities_), end(plugin.incompatibilities_)); @@ -92,14 +87,9 @@ PluginMetadata PluginMetadata::NewMetadata(const PluginMetadata& plugin) const { } // Compare this plugin against the given plugin. - set filesDiff; - set_difference(begin(loadAfter_), - end(loadAfter_), - begin(plugin.loadAfter_), - end(plugin.loadAfter_), - inserter(filesDiff, begin(filesDiff))); - p.SetLoadAfterFiles(filesDiff); + p.SetLoadAfterFiles(diffVectors(loadAfter_, plugin.loadAfter_)); + set filesDiff; filesDiff.clear(); set_difference(begin(requirements_), end(requirements_), @@ -167,7 +157,9 @@ std::string PluginMetadata::GetName() const { return name_; } std::optional PluginMetadata::GetGroup() const { return group_; } -std::set PluginMetadata::GetLoadAfterFiles() const { return loadAfter_; } +std::vector PluginMetadata::GetLoadAfterFiles() const { + return loadAfter_; +} std::set PluginMetadata::GetRequirements() const { return requirements_; } @@ -202,15 +194,11 @@ std::vector PluginMetadata::GetSimpleMessages( return simpleMessages; } -void PluginMetadata::SetGroup(const std::string& group) { - group_ = group; -} +void PluginMetadata::SetGroup(const std::string& group) { group_ = group; } -void PluginMetadata::UnsetGroup() { - group_ = std::nullopt; -} +void PluginMetadata::UnsetGroup() { group_ = std::nullopt; } -void PluginMetadata::SetLoadAfterFiles(const std::set& l) { +void PluginMetadata::SetLoadAfterFiles(const std::vector& l) { loadAfter_ = l; } @@ -242,8 +230,7 @@ void PluginMetadata::SetLocations(const std::set& locations) { } bool PluginMetadata::HasNameOnly() const { - return !group_.has_value() && - loadAfter_.empty() && requirements_.empty() && + return !group_.has_value() && loadAfter_.empty() && requirements_.empty() && incompatibilities_.empty() && messages_.empty() && tags_.empty() && dirtyInfo_.empty() && cleanInfo_.empty() && locations_.empty(); } diff --git a/src/api/metadata/yaml/plugin_metadata.h b/src/api/metadata/yaml/plugin_metadata.h index 508947e8..a5c486b6 100644 --- a/src/api/metadata/yaml/plugin_metadata.h +++ b/src/api/metadata/yaml/plugin_metadata.h @@ -103,7 +103,7 @@ struct convert { rhs.SetGroup(node["group"].as()); if (node["after"]) - rhs.SetLoadAfterFiles(node["after"].as>()); + rhs.SetLoadAfterFiles(node["after"].as>()); if (node["req"]) rhs.SetRequirements(node["req"].as>()); if (node["inc"]) diff --git a/src/api/sorting/plugin_sorting_data.cpp b/src/api/sorting/plugin_sorting_data.cpp index 529bf3e0..2bda785b 100644 --- a/src/api/sorting/plugin_sorting_data.cpp +++ b/src/api/sorting/plugin_sorting_data.cpp @@ -85,7 +85,7 @@ PluginSortingData::PluginSortingData( } else { // Not all masters are loaded, fall back to using the plugin's // total record count (Morrowind doesn't have groups). This is OK - // because plugins with missing masters can't be loaded by the game, + // because plugins with missing masters can't be loaded by the game, // so the correctness of their load order positions is less important // (it may not matter at all, depending on the sophistication/usage of // merge patches in Morrowind). It's better for LOOT to sort a load @@ -134,11 +134,11 @@ void PluginSortingData::SetAfterGroupPlugins( afterGroupPlugins_ = plugins; } -const std::set& PluginSortingData::GetMasterlistLoadAfterFiles() const { +const std::vector& PluginSortingData::GetMasterlistLoadAfterFiles() const { return masterlistLoadAfter_; } -const std::set& PluginSortingData::GetUserLoadAfterFiles() const { +const std::vector& PluginSortingData::GetUserLoadAfterFiles() const { return userLoadAfter_; } diff --git a/src/api/sorting/plugin_sorting_data.h b/src/api/sorting/plugin_sorting_data.h index 198bb8a9..599196a7 100644 --- a/src/api/sorting/plugin_sorting_data.h +++ b/src/api/sorting/plugin_sorting_data.h @@ -50,8 +50,8 @@ public: std::unordered_set GetAfterGroupPlugins() const; void SetAfterGroupPlugins(std::unordered_set plugins); - const std::set& GetMasterlistLoadAfterFiles() const; - const std::set& GetUserLoadAfterFiles() const; + const std::vector& GetMasterlistLoadAfterFiles() const; + const std::vector& GetUserLoadAfterFiles() const; const std::set& GetMasterlistRequirements() const; const std::set& GetUserRequirements() const; @@ -62,8 +62,8 @@ private: std::string group_; std::unordered_set afterGroupPlugins_; - std::set masterlistLoadAfter_; - std::set userLoadAfter_; + std::vector masterlistLoadAfter_; + std::vector userLoadAfter_; std::set masterlistReq_; std::set userReq_; diff --git a/src/tests/api/interface/database_interface_test.h b/src/tests/api/interface/database_interface_test.h index 3057f470..124a61d6 100644 --- a/src/tests/api/interface/database_interface_test.h +++ b/src/tests/api/interface/database_interface_test.h @@ -559,9 +559,9 @@ TEST_P( auto metadata = db_->GetPluginMetadata(blankEsm, true).value(); - std::set expectedLoadAfter({ - File(masterFile), + std::vector expectedLoadAfter({ File(blankDifferentEsm), + File(masterFile), }); EXPECT_EQ(expectedLoadAfter, metadata.GetLoadAfterFiles()); } @@ -596,7 +596,7 @@ TEST_P( auto metadata = db_->GetPluginMetadata(blankEsm, false).value(); - std::set expectedLoadAfter({ + std::vector expectedLoadAfter({ File(masterFile), }); EXPECT_EQ(expectedLoadAfter, metadata.GetLoadAfterFiles()); @@ -631,7 +631,7 @@ TEST_P(DatabaseInterfaceTest, auto metadata = db_->GetPluginUserMetadata(blankEsm).value(); - std::set expectedLoadAfter({ + std::vector expectedLoadAfter({ File(blankDifferentEsm), }); EXPECT_EQ(expectedLoadAfter, metadata.GetLoadAfterFiles()); @@ -683,7 +683,7 @@ TEST_P(DatabaseInterfaceTest, auto metadata = db_->GetPluginMetadata(blankEsm).value(); - std::set expectedLoadAfter({ + std::vector expectedLoadAfter({ File(masterFile), }); EXPECT_EQ(expectedLoadAfter, metadata.GetLoadAfterFiles()); @@ -711,7 +711,7 @@ TEST_P( auto metadata = db_->GetPluginMetadata(blankEsm).value(); - std::set expectedLoadAfter({ + std::vector expectedLoadAfter({ File(masterFile), }); EXPECT_EQ(expectedLoadAfter, metadata.GetLoadAfterFiles()); @@ -781,7 +781,7 @@ TEST_P( auto metadata = db_->GetPluginMetadata(blankEsm).value(); - std::set expectedLoadAfter({ + std::vector expectedLoadAfter({ File(masterFile), }); EXPECT_EQ(expectedLoadAfter, metadata.GetLoadAfterFiles()); diff --git a/src/tests/api/internals/metadata/condition_evaluator_test.h b/src/tests/api/internals/metadata/condition_evaluator_test.h index ce70b7f5..8f336661 100644 --- a/src/tests/api/internals/metadata/condition_evaluator_test.h +++ b/src/tests/api/internals/metadata/condition_evaluator_test.h @@ -189,11 +189,12 @@ TEST_P(ConditionEvaluatorTest, evaluateAllShouldEvaluateAllMetadataConditions) { EXPECT_NO_THROW(plugin = evaluator_.EvaluateAll(plugin)); - std::set expectedFiles({file1}); + std::vector expectedFiles({file1}); + std::set expectedFileSet({file1}); EXPECT_EQ("group1", plugin.GetGroup().value()); EXPECT_EQ(expectedFiles, plugin.GetLoadAfterFiles()); - EXPECT_EQ(expectedFiles, plugin.GetRequirements()); - EXPECT_EQ(expectedFiles, plugin.GetIncompatibilities()); + EXPECT_EQ(expectedFileSet, plugin.GetRequirements()); + EXPECT_EQ(expectedFileSet, plugin.GetIncompatibilities()); EXPECT_EQ(std::vector({message1}), plugin.GetMessages()); EXPECT_EQ(std::set({tag1}), plugin.GetTags()); EXPECT_EQ(std::set({info1}), plugin.GetDirtyInfo()); diff --git a/src/tests/api/internals/metadata/plugin_metadata_test.h b/src/tests/api/internals/metadata/plugin_metadata_test.h index 38dae6fd..c5af3142 100644 --- a/src/tests/api/internals/metadata/plugin_metadata_test.h +++ b/src/tests/api/internals/metadata/plugin_metadata_test.h @@ -169,7 +169,7 @@ TEST_P(PluginMetadataTest, mergeMetadataShouldMergeLoadAfterData) { plugin2.SetLoadAfterFiles({file1, file2}); plugin1.MergeMetadata(plugin2); - EXPECT_EQ(std::set({file1, file2}), plugin1.GetLoadAfterFiles()); + EXPECT_EQ(std::vector({file1, file2}), plugin1.GetLoadAfterFiles()); } TEST_P(PluginMetadataTest, mergeMetadataShouldMergeRequirementData) { @@ -324,7 +324,7 @@ TEST_P(PluginMetadataTest, plugin2.SetLoadAfterFiles({file1, file3}); PluginMetadata newMetadata = plugin1.NewMetadata(plugin2); - EXPECT_EQ(std::set({file2}), newMetadata.GetLoadAfterFiles()); + EXPECT_EQ(std::vector({file2}), newMetadata.GetLoadAfterFiles()); } TEST_P( @@ -785,7 +785,7 @@ TEST_P(PluginMetadataTest, YAML::Node node; node = plugin; - EXPECT_EQ(plugin.GetLoadAfterFiles(), node["after"].as>()); + EXPECT_EQ(plugin.GetLoadAfterFiles(), node["after"].as>()); } TEST_P(PluginMetadataTest, encodingAsYamlShouldSetReqFieldIfRequirementsExist) { @@ -881,7 +881,7 @@ TEST_P(PluginMetadataTest, decodingFromYamlShouldStoreAllGivenData) { PluginMetadata plugin = node.as(); EXPECT_EQ("Blank.esp", plugin.GetName()); - EXPECT_EQ(std::set({File("Blank.esm")}), plugin.GetLoadAfterFiles()); + EXPECT_EQ(std::vector({File("Blank.esm")}), plugin.GetLoadAfterFiles()); EXPECT_EQ(std::set({File("Blank.esm")}), plugin.GetRequirements()); EXPECT_EQ(std::set({File("Blank.esm")}), plugin.GetIncompatibilities()); EXPECT_EQ(std::vector({Message(MessageType::say, "content")}), diff --git a/src/tests/api/internals/metadata_list_test.h b/src/tests/api/internals/metadata_list_test.h index 4908c1f9..04a57465 100644 --- a/src/tests/api/internals/metadata_list_test.h +++ b/src/tests/api/internals/metadata_list_test.h @@ -293,7 +293,7 @@ TEST_P( PluginMetadata plugin = metadataList.FindPlugin(blankDifferentEsp).value(); EXPECT_EQ(blankDifferentEsp, plugin.GetName()); - EXPECT_EQ(std::set({ + EXPECT_EQ(std::vector({ File(blankEsm), }), plugin.GetLoadAfterFiles()); diff --git a/src/tests/api/internals/sorting/plugin_sort_test.h b/src/tests/api/internals/sorting/plugin_sort_test.h index 590a414f..df26add4 100644 --- a/src/tests/api/internals/sorting/plugin_sort_test.h +++ b/src/tests/api/internals/sorting/plugin_sort_test.h @@ -240,7 +240,7 @@ TEST_P( PluginMetadata plugin(blankEsp); plugin = PluginMetadata(blankDifferentMasterDependentEsp); - plugin.SetLoadAfterFiles(std::set({File(blankMasterDependentEsp)})); + plugin.SetLoadAfterFiles({File(blankMasterDependentEsp)}); game_.GetDatabase()->SetPluginUserMetadata(plugin); plugin = PluginMetadata(blankDifferentEsp);