From c74f2f4978ea2fdd94b24fa3bf294dc5f0192315 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Thu, 11 Jun 2020 09:48:27 +0100 Subject: [PATCH] Remove PluginMetadata::operator== and != for PluginMetadata Unlike the other metadata classes, they were only comparing plugin names not full object equality. Checking full object equality isn't very useful, so instead the operators have been replaced by a new NameMatches() method that takes a non-regex plugin name string. --- include/loot/metadata/plugin_metadata.h | 22 +++++----- src/api/metadata/plugin_metadata.cpp | 19 +++----- src/api/metadata_list.cpp | 23 ++++++---- src/api/metadata_list.h | 24 ++++------- .../internals/metadata/plugin_metadata_test.h | 43 +++++++------------ 5 files changed, 53 insertions(+), 78 deletions(-) diff --git a/include/loot/metadata/plugin_metadata.h b/include/loot/metadata/plugin_metadata.h index 44b4e671..7bcaca10 100644 --- a/include/loot/metadata/plugin_metadata.h +++ b/include/loot/metadata/plugin_metadata.h @@ -236,20 +236,18 @@ public: LOOT_API bool IsRegexPlugin() const; /** - * Check if two PluginMetadata objects are equal by comparing their name - * values. - * @returns True if the plugin names are case-insensitively equal, false - * otherwise. + * Check if the given plugin name matches this plugin metadata object's + * name field. + * + * If the name field is a regular expression, the given plugin name will be + * matched against it, otherwise the strings will be compared + * case-insensitively. The given plugin name must be literal, i.e. not a + * regular expression. + * @returns True if the given plugin name matches this metadata's plugin + * name, false otherwise. */ - LOOT_API bool operator==(const PluginMetadata& rhs) const; + LOOT_API bool NameMatches(const std::string& pluginName) const; - /** - * Check if two PluginMetadata objects are not equal by comparing their name - * values. - * @returns True if the plugin names are not case-insensitively equal, false - * otherwise. - */ - LOOT_API bool operator!=(const PluginMetadata& rhs) const; private: std::string name_; std::optional group_; diff --git a/src/api/metadata/plugin_metadata.cpp b/src/api/metadata/plugin_metadata.cpp index 4a4af8ca..c7dafaa4 100644 --- a/src/api/metadata/plugin_metadata.cpp +++ b/src/api/metadata/plugin_metadata.cpp @@ -199,20 +199,13 @@ bool PluginMetadata::IsRegexPlugin() const { return strpbrk(name_.c_str(), ":\\*?|") != nullptr; } -bool PluginMetadata::operator==(const PluginMetadata& rhs) const { - if (IsRegexPlugin() == rhs.IsRegexPlugin()) { - return CompareFilenames(name_, rhs.name_) == 0; +bool PluginMetadata::NameMatches(const std::string& pluginName) const { + if (IsRegexPlugin()) { + return std::regex_match( + pluginName, + std::regex(name_, std::regex::ECMAScript | std::regex::icase)); } - if (IsRegexPlugin()) - return regex_match(rhs.GetName(), - regex(name_, regex::ECMAScript | regex::icase)); - else - return regex_match(name_, - regex(rhs.GetName(), regex::ECMAScript | regex::icase)); -} - -bool PluginMetadata::operator!=(const PluginMetadata& rhs) const { - return !(*this == rhs); + return CompareFilenames(name_, pluginName) == 0; } } diff --git a/src/api/metadata_list.cpp b/src/api/metadata_list.cpp index 7e96e47b..6fff9bf0 100644 --- a/src/api/metadata_list.cpp +++ b/src/api/metadata_list.cpp @@ -60,7 +60,7 @@ void MetadataList::Load(const std::filesystem::path& filepath) { PluginMetadata plugin(node.as()); if (plugin.IsRegexPlugin()) regexPlugins_.push_back(plugin); - else if (!plugins_.insert(plugin).second) + else if (!plugins_.emplace(Filename(plugin.GetName()), plugin).second) throw FileAccessError("More than one entry exists for plugin \"" + plugin.GetName() + "\""); } @@ -158,7 +158,9 @@ void MetadataList::Clear() { std::vector MetadataList::Plugins() const { std::vector plugins; plugins.reserve(plugins_.size() + regexPlugins_.size()); - plugins.insert(plugins.end(), plugins_.begin(), plugins_.end()); + for (const auto& pluginPair : plugins_) { + plugins.push_back(pluginPair.second); + } plugins.insert(plugins.end(), regexPlugins_.begin(), regexPlugins_.end()); return plugins; @@ -198,17 +200,20 @@ std::optional MetadataList::FindPlugin( const std::string& pluginName) const { PluginMetadata match(pluginName); - auto it = plugins_.find(match); + auto it = plugins_.find(Filename(pluginName)); if (it != plugins_.end()) - match = *it; + match = it->second; // Now we want to also match possibly multiple regex entries. - auto regIt = find(regexPlugins_.begin(), regexPlugins_.end(), match); + auto nameMatches = [&](const PluginMetadata& pluginMetadata) { + return pluginMetadata.NameMatches(pluginName); + }; + auto regIt = find_if(regexPlugins_.begin(), regexPlugins_.end(), nameMatches); while (regIt != regexPlugins_.end()) { match.MergeMetadata(*regIt); - regIt = find(++regIt, regexPlugins_.end(), match); + regIt = find_if(++regIt, regexPlugins_.end(), nameMatches); } if (match.HasNameOnly()) { @@ -222,7 +227,7 @@ void MetadataList::AddPlugin(const PluginMetadata& plugin) { if (plugin.IsRegexPlugin()) regexPlugins_.push_back(plugin); else { - if (!plugins_.insert(plugin).second) + if (!plugins_.emplace(Filename(plugin.GetName()), plugin).second) throw std::invalid_argument( "Cannot add \"" + plugin.GetName() + "\" to the metadata list as another entry already exists."); @@ -232,7 +237,7 @@ void MetadataList::AddPlugin(const PluginMetadata& plugin) { // Doesn't erase matching regex entries, because they might also // be required for other plugins. void MetadataList::ErasePlugin(const std::string& pluginName) { - auto it = plugins_.find(PluginMetadata(pluginName)); + auto it = plugins_.find(Filename(pluginName)); if (it != plugins_.end()) { plugins_.erase(it); @@ -251,7 +256,7 @@ void MetadataList::EvalAllConditions(ConditionEvaluator& conditionEvaluator) { plugins_.clear(); for (const auto& plugin : unevaluatedPlugins_) { - plugins_.insert(conditionEvaluator.EvaluateAll(plugin)); + plugins_.emplace(plugin.first, conditionEvaluator.EvaluateAll(plugin.second)); } if (unevaluatedRegexPlugins_.empty()) diff --git a/src/api/metadata_list.h b/src/api/metadata_list.h index ad3abad8..4947fc16 100644 --- a/src/api/metadata_list.h +++ b/src/api/metadata_list.h @@ -37,20 +37,12 @@ #include "loot/metadata/plugin_metadata.h" namespace std { - /** - * A specialisation of std::hash for loot::PluginMetadata. - */ - template<> - struct hash { - /** - * Calculate a hash value for an object of a class that implements - * loot::PluginMetadata. - * @return The hash generated from the plugin's normalized filename. - */ - size_t operator()(const loot::PluginMetadata& plugin) const { - return hash()(loot::NormalizeFilename(plugin.GetName())); - } - }; +template<> +struct hash { + size_t operator()(const loot::Filename& filename) const { + return hash()(loot::NormalizeFilename(std::string(filename))); + } +}; } namespace loot { @@ -83,11 +75,11 @@ public: protected: std::vector groups_; std::vector bashTags_; - std::unordered_set plugins_; + std::unordered_map plugins_; std::vector regexPlugins_; std::vector messages_; - std::unordered_set unevaluatedPlugins_; + std::unordered_map unevaluatedPlugins_; std::vector unevaluatedRegexPlugins_; std::vector unevaluatedMessages_; }; diff --git a/src/tests/api/internals/metadata/plugin_metadata_test.h b/src/tests/api/internals/metadata/plugin_metadata_test.h index cb839ccc..5a93b63c 100644 --- a/src/tests/api/internals/metadata/plugin_metadata_test.h +++ b/src/tests/api/internals/metadata/plugin_metadata_test.h @@ -68,40 +68,26 @@ TEST_P( } TEST_P(PluginMetadataTest, - equalityOperatorShouldUseCaseInsensitiveNameComparisonForNonRegexNames) { - PluginMetadata plugin1(blankEsm); - PluginMetadata plugin2(boost::to_lower_copy(blankEsm)); - EXPECT_TRUE(plugin1 == plugin2); + nameMatchesShouldUseCaseInsensitiveNameComparisonForNonRegexNames) { + PluginMetadata plugin(blankEsm); - plugin1 = PluginMetadata(blankEsm); - plugin2 = PluginMetadata(blankDifferentEsm); - EXPECT_FALSE(plugin1 == plugin2); + EXPECT_TRUE(plugin.NameMatches(boost::to_lower_copy(blankEsm))); + EXPECT_FALSE(plugin.NameMatches(blankDifferentEsm)); } TEST_P(PluginMetadataTest, - equalityOperatorShouldUseCaseInsensitiveNameComparisonForTwoRegexNames) { - PluginMetadata plugin1("Blan.\\.esm"); - PluginMetadata plugin2("blan.\\.esm"); - EXPECT_TRUE(plugin1 == plugin2); - EXPECT_TRUE(plugin2 == plugin1); + nameMatchesShouldTreatGivenPluginNameStringsAsLiterals) { + PluginMetadata plugin(blankEsm); + std::string regex = "blan.\\.esm"; - plugin1 = PluginMetadata("Blan(k|p).esm"); - plugin2 = PluginMetadata("Blan.\\.esm"); - EXPECT_FALSE(plugin1 == plugin2); - EXPECT_FALSE(plugin2 == plugin1); + EXPECT_FALSE(plugin.NameMatches(regex)); } -TEST_P(PluginMetadataTest, - equalityOperatorShouldUseRegexMatchingForARegexNameAndANonRegexName) { - PluginMetadata plugin1("Blank.esm"); - PluginMetadata plugin2("Blan.\\.esm"); - EXPECT_TRUE(plugin1 == plugin2); - EXPECT_TRUE(plugin2 == plugin1); +TEST_P(PluginMetadataTest, nameMatchesShouldUseCaseInsensitiveRegexMatchingForARegexName) { + PluginMetadata plugin("Blan.\\.esm"); - plugin1 = PluginMetadata("Blan.esm"); - plugin2 = PluginMetadata("Blan.\\.esm"); - EXPECT_FALSE(plugin1 == plugin2); - EXPECT_FALSE(plugin2 == plugin1); + EXPECT_TRUE(plugin.NameMatches(boost::to_lower_copy(blankEsm))); + EXPECT_FALSE(plugin.NameMatches(blankDifferentEsm)); } TEST_P(PluginMetadataTest, mergeMetadataShouldNotChangeName) { @@ -114,7 +100,7 @@ TEST_P(PluginMetadataTest, mergeMetadataShouldNotChangeName) { } TEST_P(PluginMetadataTest, - mergeMetadataShouldNotUseMergedGroupIfItAndCurrentGroupAreBothExplicit) { + mergeMetadataShouldNotUseMergedGroupIfItAndCurrentGroupAreBothExplicit) { PluginMetadata plugin1; PluginMetadata plugin2; @@ -886,7 +872,8 @@ TEST_P(PluginMetadataTest, decodingFromYamlShouldStoreAllGivenData) { EXPECT_EQ("Blank.esp", plugin.GetName()); EXPECT_EQ(std::vector({File("Blank.esm")}), plugin.GetLoadAfterFiles()); EXPECT_EQ(std::vector({File("Blank.esm")}), plugin.GetRequirements()); - EXPECT_EQ(std::vector({File("Blank.esm")}), plugin.GetIncompatibilities()); + EXPECT_EQ(std::vector({File("Blank.esm")}), + plugin.GetIncompatibilities()); EXPECT_EQ(std::vector({Message(MessageType::say, "content")}), plugin.GetMessages()); EXPECT_EQ(std::vector({Tag("Relev")}), plugin.GetTags());