From ac7d60cc764228bf8034e20d55a7a5f2643f548d Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sat, 19 Feb 2022 14:03:35 +0000 Subject: [PATCH] Replace reference member with a pointer As recommended by C++ Core Guidelines C.12. This doesn't change the lifetime requirements of the class, but I've added a comment to highlight them. --- src/api/sorting/plugin_graph.cpp | 2 +- src/api/sorting/plugin_sorting_data.cpp | 25 ++++++------- src/api/sorting/plugin_sorting_data.h | 9 +++-- .../sorting/plugin_sorting_data_test.h | 36 +++++++++---------- 4 files changed, 39 insertions(+), 33 deletions(-) diff --git a/src/api/sorting/plugin_graph.cpp b/src/api/sorting/plugin_graph.cpp index dfb7db23..fe85acc8 100644 --- a/src/api/sorting/plugin_graph.cpp +++ b/src/api/sorting/plugin_graph.cpp @@ -215,7 +215,7 @@ void PluginGraph::AddPluginVertices(Game& game, .GetPluginUserMetadata(plugin->GetName(), true) .value_or(PluginMetadata(plugin->GetName())); - auto pluginSortingData = PluginSortingData(*plugin, + auto pluginSortingData = PluginSortingData(plugin, masterlistMetadata, userMetadata, loadOrder, diff --git a/src/api/sorting/plugin_sorting_data.cpp b/src/api/sorting/plugin_sorting_data.cpp index 1148dd41..722a961c 100644 --- a/src/api/sorting/plugin_sorting_data.cpp +++ b/src/api/sorting/plugin_sorting_data.cpp @@ -50,7 +50,7 @@ std::vector GetPluginsSubset( } PluginSortingData::PluginSortingData( - const Plugin& plugin, + const Plugin* plugin, const PluginMetadata& masterlistMetadata, const PluginMetadata& userMetadata, const std::vector& loadOrder, @@ -70,19 +70,19 @@ PluginSortingData::PluginSortingData( } for (size_t i = 0; i < loadOrder.size(); i++) { - if (CompareFilenames(plugin.GetName(), loadOrder.at(i)) == 0) { + if (CompareFilenames(plugin->GetName(), loadOrder.at(i)) == 0) { loadOrderIndex_ = i; } } if (gameType == GameType::tes3) { - auto masterNames = plugin.GetMasters(); + auto masterNames = plugin->GetMasters(); if (masterNames.empty()) { numOverrideFormIDs = 0; } else { auto masters = GetPluginsSubset(loadedPlugins, masterNames); if (masters.size() == masterNames.size()) { - numOverrideFormIDs = plugin.GetOverlapSize(masters); + numOverrideFormIDs = plugin->GetOverlapSize(masters); } else { // Not all masters are loaded, fall back to using the plugin's // total record count (Morrowind doesn't have groups). This is OK @@ -93,25 +93,26 @@ PluginSortingData::PluginSortingData( // order with missing masters with potentially poorer results than // for it to error out, as masters may be missing for a variety of // development & testing reasons. - numOverrideFormIDs = plugin.GetRecordAndGroupCount(); + numOverrideFormIDs = plugin->GetRecordAndGroupCount(); } } } else { - numOverrideFormIDs = plugin.NumOverrideFormIDs(); + numOverrideFormIDs = plugin->NumOverrideFormIDs(); } } -std::string PluginSortingData::GetName() const { return plugin_.GetName(); } +std::string PluginSortingData::GetName() const { return plugin_->GetName(); } bool PluginSortingData::IsMaster() const { - return plugin_.IsMaster() || (plugin_.IsLightPlugin() && - !boost::iends_with(plugin_.GetName(), ".esp")); + return plugin_->IsMaster() || + (plugin_->IsLightPlugin() && + !boost::iends_with(plugin_->GetName(), ".esp")); } -bool PluginSortingData::LoadsArchive() const { return plugin_.LoadsArchive(); } +bool PluginSortingData::LoadsArchive() const { return plugin_->LoadsArchive(); } std::vector PluginSortingData::GetMasters() const { - return plugin_.GetMasters(); + return plugin_->GetMasters(); } size_t PluginSortingData::NumOverrideFormIDs() const { @@ -120,7 +121,7 @@ size_t PluginSortingData::NumOverrideFormIDs() const { bool PluginSortingData::DoFormIDsOverlap( const PluginSortingData& plugin) const { - return plugin_.DoFormIDsOverlap(plugin.plugin_); + return plugin_->DoFormIDsOverlap(*plugin.plugin_); } std::string PluginSortingData::GetGroup() const { return group_; } diff --git a/src/api/sorting/plugin_sorting_data.h b/src/api/sorting/plugin_sorting_data.h index 6f2d6d88..7b4bfe77 100644 --- a/src/api/sorting/plugin_sorting_data.h +++ b/src/api/sorting/plugin_sorting_data.h @@ -31,7 +31,12 @@ namespace loot { class PluginSortingData { public: - explicit PluginSortingData(const Plugin& plugin, + /** + * This stores a copy of the plugin pointer that is passed to it, so + * PluginSortingData objects must not live longer than the Plugin objects + * that they are constructed from. + */ + explicit PluginSortingData(const Plugin* plugin, const PluginMetadata& masterlistMetadata, const PluginMetadata& userMetadata, const std::vector& loadOrder, @@ -58,7 +63,7 @@ public: const std::optional& GetLoadOrderIndex() const; private: - const Plugin& plugin_; + const Plugin* plugin_; std::string group_; std::unordered_set afterGroupPlugins_; diff --git a/src/tests/api/internals/sorting/plugin_sorting_data_test.h b/src/tests/api/internals/sorting/plugin_sorting_data_test.h index 4a85da0b..5841ae01 100644 --- a/src/tests/api/internals/sorting/plugin_sorting_data_test.h +++ b/src/tests/api/internals/sorting/plugin_sorting_data_test.h @@ -84,27 +84,27 @@ TEST_P(PluginSortingDataTest, lightFlaggedEspFilesShouldNotBeTreatedAsMasters) { ASSERT_NO_THROW(loadInstalledPlugins(game_, false)); - auto esp = PluginSortingData( - *dynamic_cast(game_.GetPlugin(blankEsp)), - PluginMetadata(), - PluginMetadata(), - getLoadOrder(), - game_.Type(), - game_.GetCache().GetPlugins()); + auto esp = + PluginSortingData(dynamic_cast(game_.GetPlugin(blankEsp)), + PluginMetadata(), + PluginMetadata(), + getLoadOrder(), + game_.Type(), + game_.GetCache().GetPlugins()); EXPECT_FALSE(esp.IsMaster()); - auto master = PluginSortingData( - *dynamic_cast(game_.GetPlugin(blankEsm)), - PluginMetadata(), - PluginMetadata(), - getLoadOrder(), - game_.Type(), - game_.GetCache().GetPlugins()); + auto master = + PluginSortingData(dynamic_cast(game_.GetPlugin(blankEsm)), + PluginMetadata(), + PluginMetadata(), + getLoadOrder(), + game_.Type(), + game_.GetCache().GetPlugins()); EXPECT_TRUE(master.IsMaster()); if (GetParam() == GameType::fo4 || GetParam() == GameType::tes5se) { auto lightMaster = PluginSortingData( - *dynamic_cast(game_.GetPlugin(blankEsl)), + dynamic_cast(game_.GetPlugin(blankEsl)), PluginMetadata(), PluginMetadata(), getLoadOrder(), @@ -113,7 +113,7 @@ TEST_P(PluginSortingDataTest, lightFlaggedEspFilesShouldNotBeTreatedAsMasters) { EXPECT_TRUE(lightMaster.IsMaster()); auto lightPlugin = PluginSortingData( - *dynamic_cast(game_.GetPlugin(blankEslEsp)), + dynamic_cast(game_.GetPlugin(blankEslEsp)), PluginMetadata(), PluginMetadata(), getLoadOrder(), @@ -128,7 +128,7 @@ TEST_P(PluginSortingDataTest, ASSERT_NO_THROW(loadInstalledPlugins(game_, false)); auto plugin = PluginSortingData( - *dynamic_cast(game_.GetPlugin(blankMasterDependentEsm)), + dynamic_cast(game_.GetPlugin(blankMasterDependentEsm)), PluginMetadata(), PluginMetadata(), getLoadOrder(), @@ -157,7 +157,7 @@ TEST_P( } auto plugin = PluginSortingData( - *dynamic_cast(game_.GetPlugin(blankMasterDependentEsm)), + dynamic_cast(game_.GetPlugin(blankMasterDependentEsm)), PluginMetadata(), PluginMetadata(), getLoadOrder(),