Replace unnecessary usage of std::shared_ptr

- CreateGameHandle() should return a unique_ptr because it transfers
  ownership.
- GameInterface::GetDatabase() should return a reference because it
  returns an existing object that will always be valid at call time.
- GameInterface::GetPlugin() and GameInterface::GetLoadedPlugins()
  return raw pointers because they do not transfer or share ownership.
  It's unfortunately that references can't be used instead (ignoring
  std::reference_wrapper), as std::optional<const Plugin&> and
  std::vector<const Plugin&> would be more meaningful return types.

Internally, there were a few uses of shared_ptr that could be unique_ptr
and a few that could be non-pointer types.

A few uses of shared_ptr remain:

- Game and ApiDatabase share a ConditionEvaluator, so it's kept inside a
  shared_ptr. Technically ApiDatabase is used such that the
  ConditionEvaluator it uses will always outlive it, but that's not
  guaranteed, so shared_ptr is used for safety.
- Plugin objects are cached inside shared_ptr so that the map of
  them can be iterated over. Ideally they'd be stored in unique_ptr,
  but unique_ptr not being copyable means the map entries can't be
  iterated over.
This commit is contained in:
Oliver Hamlet
2022-02-18 22:29:08 +00:00
parent 28102c1aa2
commit bfb168907c
27 changed files with 325 additions and 308 deletions
+1 -1
View File
@@ -105,7 +105,7 @@ LOOT_API bool IsCompatible(const unsigned int major,
* variable (eg. Linux) can still use the API.
* @returns The new game handle.
*/
LOOT_API std::shared_ptr<GameInterface> CreateGameHandle(
LOOT_API std::unique_ptr<GameInterface> CreateGameHandle(
const GameType game,
const std::filesystem::path& game_path,
const std::filesystem::path& game_local_path = "");
+7 -5
View File
@@ -33,6 +33,8 @@ namespace loot {
/** @brief The interface provided for accessing game-specific functionality. */
class GameInterface {
public:
virtual ~GameInterface() = default;
/**
* @name Metadata Access
* @{
@@ -41,9 +43,10 @@ public:
/**
* @brief Get the database interface used for accessing metadata-related
* functionality.
* @returns A shared pointer to the game's DatabaseInterface
* @returns A reference to the game's DatabaseInterface. The reference remains
* valid for the lifetime of the GameInterface instance.
*/
virtual std::shared_ptr<DatabaseInterface> GetDatabase() = 0;
virtual DatabaseInterface& GetDatabase() = 0;
/**
* @}
@@ -84,7 +87,7 @@ public:
* @returns A shared pointer to a const PluginInterface implementation. The
* pointer is null if the given plugin has not been loaded.
*/
virtual std::shared_ptr<const PluginInterface> GetPlugin(
virtual const PluginInterface* GetPlugin(
const std::string& pluginName) const = 0;
/**
@@ -94,8 +97,7 @@ public:
* valid until the ``LoadPlugins()`` or ``SortPlugins()`` functions
* are next called or this GameInterface is destroyed.
*/
virtual std::vector<std::shared_ptr<const PluginInterface>> GetLoadedPlugins()
const = 0;
virtual std::vector<const PluginInterface*> GetLoadedPlugins() const = 0;
/**
* @}
+2 -2
View File
@@ -72,7 +72,7 @@ LOOT_API bool IsCompatible(const unsigned int versionMajor,
return versionMinor == loot::LootVersion::minor;
}
LOOT_API std::shared_ptr<GameInterface> CreateGameHandle(
LOOT_API std::unique_ptr<GameInterface> CreateGameHandle(
const GameType game,
const std::filesystem::path& gamePath,
const std::filesystem::path& gameLocalPath) {
@@ -96,6 +96,6 @@ LOOT_API std::shared_ptr<GameInterface> CreateGameHandle(
gameLocalPath.u8string() +
"\" does not resolve to a valid directory.");
return std::make_shared<Game>(game, resolvedGamePath, resolvedGameLocalPath);
return std::make_unique<Game>(game, resolvedGamePath, resolvedGameLocalPath);
}
}
-2
View File
@@ -29,8 +29,6 @@
#include <string>
#include <vector>
#include "api/game/game_cache.h"
#include "api/game/load_order_handler.h"
#include "api/metadata/condition_evaluator.h"
#include "api/metadata_list.h"
#include "loot/database_interface.h"
+21 -30
View File
@@ -59,8 +59,9 @@ Game::Game(const GameType gameType,
const std::filesystem::path& localDataPath) :
type_(gameType),
gamePath_(gamePath),
cache_(std::make_shared<GameCache>()),
loadOrderHandler_(std::make_shared<LoadOrderHandler>()) {
conditionEvaluator_(
std::make_shared<ConditionEvaluator>(Type(), DataPath())),
database_(ApiDatabase(conditionEvaluator_)) {
auto logger = getLogger();
if (logger) {
logger->info("Initialising load order data for game of type {} at: {}",
@@ -68,12 +69,7 @@ Game::Game(const GameType gameType,
gamePath_.u8string());
}
loadOrderHandler_->Init(type_, gamePath_, localDataPath);
conditionEvaluator_ =
std::make_shared<ConditionEvaluator>(Type(), DataPath());
database_ = std::make_shared<ApiDatabase>(conditionEvaluator_);
loadOrderHandler_.Init(type_, gamePath_, localDataPath);
}
GameType Game::Type() const { return type_; }
@@ -86,13 +82,11 @@ std::filesystem::path Game::DataPath() const {
}
}
std::shared_ptr<GameCache> Game::GetCache() { return cache_; }
GameCache& Game::GetCache() { return cache_; }
std::shared_ptr<LoadOrderHandler> Game::GetLoadOrderHandler() {
return loadOrderHandler_;
}
LoadOrderHandler& Game::GetLoadOrderHandler() { return loadOrderHandler_; }
std::shared_ptr<DatabaseInterface> Game::GetDatabase() { return database_; }
DatabaseInterface& Game::GetDatabase() { return database_; }
bool Game::IsValidPlugin(const std::string& plugin) const {
return Plugin::IsValid(Type(), DataPath() / u8path(plugin));
@@ -150,7 +144,7 @@ void Game::LoadPlugins(const std::vector<std::string>& plugins,
}
// Clear the existing plugin and archive caches.
cache_->ClearCachedPlugins();
cache_.ClearCachedPlugins();
// Search for and cache archives.
CacheArchives();
@@ -170,7 +164,7 @@ void Game::LoadPlugins(const std::vector<std::string>& plugins,
const bool loadHeader =
loadHeadersOnly || loot::equivalent(pluginPath, masterPath);
cache_->AddPlugin(Plugin(Type(), cache_, pluginPath, loadHeader));
cache_.AddPlugin(Plugin(Type(), cache_, pluginPath, loadHeader));
} catch (const std::exception& e) {
if (logger) {
logger->error(
@@ -192,17 +186,14 @@ void Game::LoadPlugins(const std::vector<std::string>& plugins,
conditionEvaluator_->RefreshLoadedPluginsState(GetLoadedPlugins());
}
std::shared_ptr<const PluginInterface> Game::GetPlugin(
const std::string& pluginName) const {
return cache_->GetPlugin(pluginName);
const PluginInterface* Game::GetPlugin(const std::string& pluginName) const {
return cache_.GetPlugin(pluginName);
}
std::vector<std::shared_ptr<const PluginInterface>> Game::GetLoadedPlugins()
const {
std::vector<std::shared_ptr<const PluginInterface>> interfacePointers;
for (auto& plugin : cache_->GetPlugins()) {
interfacePointers.push_back(
std::static_pointer_cast<const PluginInterface>(plugin));
std::vector<const PluginInterface*> Game::GetLoadedPlugins() const {
std::vector<const PluginInterface*> interfacePointers;
for (const auto plugin : cache_.GetPlugins()) {
interfacePointers.push_back(plugin);
}
return interfacePointers;
@@ -221,21 +212,21 @@ std::vector<std::string> Game::SortPlugins(
}
void Game::LoadCurrentLoadOrderState() {
loadOrderHandler_->LoadCurrentState();
loadOrderHandler_.LoadCurrentState();
conditionEvaluator_->RefreshActivePluginsState(
loadOrderHandler_->GetActivePlugins());
loadOrderHandler_.GetActivePlugins());
}
bool Game::IsPluginActive(const std::string& pluginName) const {
return loadOrderHandler_->IsPluginActive(pluginName);
return loadOrderHandler_.IsPluginActive(pluginName);
}
std::vector<std::string> Game::GetLoadOrder() const {
return loadOrderHandler_->GetLoadOrder();
return loadOrderHandler_.GetLoadOrder();
}
void Game::SetLoadOrder(const std::vector<std::string>& loadOrder) {
loadOrderHandler_->SetLoadOrder(loadOrder);
loadOrderHandler_.SetLoadOrder(loadOrder);
}
void Game::CacheArchives() {
@@ -255,6 +246,6 @@ void Game::CacheArchives() {
}
}
cache_->CacheArchivePaths(std::move(archivePaths));
cache_.CacheArchivePaths(std::move(archivePaths));
}
}
+9 -9
View File
@@ -28,6 +28,7 @@
#include <filesystem>
#include <string>
#include "api/api_database.h"
#include "api/game/game_cache.h"
#include "api/game/load_order_handler.h"
#include "api/metadata/condition_evaluator.h"
@@ -46,24 +47,23 @@ public:
GameType Type() const;
std::filesystem::path DataPath() const;
std::shared_ptr<GameCache> GetCache();
std::shared_ptr<LoadOrderHandler> GetLoadOrderHandler();
GameCache& GetCache();
LoadOrderHandler& GetLoadOrderHandler();
// Game Interface Methods //
////////////////////////////
std::shared_ptr<DatabaseInterface> GetDatabase() override;
DatabaseInterface& GetDatabase() override;
bool IsValidPlugin(const std::string& plugin) const override;
void LoadPlugins(const std::vector<std::string>& plugins,
bool loadHeadersOnly) override;
std::shared_ptr<const PluginInterface> GetPlugin(
const PluginInterface* GetPlugin(
const std::string& pluginName) const override;
std::vector<std::shared_ptr<const PluginInterface>> GetLoadedPlugins()
const override;
std::vector<const PluginInterface*> GetLoadedPlugins() const override;
void IdentifyMainMasterFile(const std::string& masterFile) override;
@@ -84,10 +84,10 @@ private:
const GameType type_;
const std::filesystem::path gamePath_;
std::shared_ptr<GameCache> cache_;
std::shared_ptr<LoadOrderHandler> loadOrderHandler_;
GameCache cache_;
LoadOrderHandler loadOrderHandler_;
std::shared_ptr<ConditionEvaluator> conditionEvaluator_;
std::shared_ptr<DatabaseInterface> database_;
ApiDatabase database_;
std::string masterFilename_;
};
+5 -6
View File
@@ -72,24 +72,23 @@ GameCache& GameCache::operator=(GameCache&& cache) {
return *this;
}
std::vector<std::shared_ptr<const Plugin>> GameCache::GetPlugins() const {
std::vector<const Plugin*> GameCache::GetPlugins() const {
lock_guard<mutex> lock(mutex_);
std::vector<std::shared_ptr<const Plugin>> output(plugins_.size());
std::vector<const Plugin*> output(plugins_.size());
std::transform(
begin(plugins_), end(plugins_), begin(output), [](const auto& pair) {
return pair.second;
return pair.second.get();
});
return output;
}
std::shared_ptr<const Plugin> GameCache::GetPlugin(
const std::string& pluginName) const {
const Plugin* GameCache::GetPlugin(const std::string& pluginName) const {
lock_guard<mutex> lock(mutex_);
const auto it = plugins_.find(NormalizeFilename(pluginName));
if (it != end(plugins_))
return it->second;
return it->second.get();
return nullptr;
}
+2 -2
View File
@@ -42,8 +42,8 @@ public:
GameCache& operator=(const GameCache& cache);
GameCache& operator=(GameCache&& cache);
std::vector<std::shared_ptr<const Plugin>> GetPlugins() const;
std::shared_ptr<const Plugin> GetPlugin(const std::string& pluginName) const;
std::vector<const Plugin*> GetPlugins() const;
const Plugin* GetPlugin(const std::string& pluginName) const;
void AddPlugin(Plugin&& plugin);
std::set<std::filesystem::path> GetArchivePaths() const;
+8 -4
View File
@@ -89,7 +89,10 @@ std::string IntToHexString(const uint32_t value) {
}
ConditionEvaluator::ConditionEvaluator(const GameType gameType,
const std::filesystem::path& dataPath) {
const std::filesystem::path& dataPath) :
lciState_(std::unique_ptr<lci_state, decltype(&lci_state_destroy)>(
nullptr,
lci_state_destroy)) {
lci_state* state = nullptr;
// This probably isn't correct for API users other than LOOT.
@@ -102,7 +105,8 @@ ConditionEvaluator::ConditionEvaluator(const GameType gameType,
lootPath.u8string().c_str());
HandleError("create state object for condition evaluation", result);
lciState_ = std::shared_ptr<lci_state>(state, lci_state_destroy);
lciState_ = std::unique_ptr<lci_state, decltype(&lci_state_destroy)>(
state, lci_state_destroy);
}
bool ConditionEvaluator::Evaluate(const std::string& condition) {
@@ -191,7 +195,7 @@ void ConditionEvaluator::ClearConditionCache() {
}
void ConditionEvaluator::RefreshActivePluginsState(
std::vector<std::string> activePluginNames) {
const std::vector<std::string>& activePluginNames) {
ClearConditionCache();
std::vector<const char*> activePluginNameCStrings;
@@ -207,7 +211,7 @@ void ConditionEvaluator::RefreshActivePluginsState(
}
void ConditionEvaluator::RefreshLoadedPluginsState(
std::vector<std::shared_ptr<const PluginInterface>> plugins) {
const std::vector<const PluginInterface*>& plugins) {
ClearConditionCache();
std::vector<std::string> pluginNames;
+7 -5
View File
@@ -28,12 +28,13 @@
#include <loot_condition_interpreter.h>
#include <filesystem>
#include <memory>
#include <string>
#include "api/game/game_cache.h"
#include "api/game/load_order_handler.h"
#include "loot/enum/game_type.h"
#include "loot/metadata/plugin_cleaning_data.h"
#include "loot/metadata/plugin_metadata.h"
#include "loot/plugin_interface.h"
namespace loot {
class ConditionEvaluator {
@@ -45,15 +46,16 @@ public:
PluginMetadata EvaluateAll(const PluginMetadata& pluginMetadata);
void ClearConditionCache();
void RefreshActivePluginsState(std::vector<std::string> activePluginNames);
void RefreshActivePluginsState(
const std::vector<std::string>& activePluginNames);
void RefreshLoadedPluginsState(
std::vector<std::shared_ptr<const PluginInterface>> plugins);
const std::vector<const PluginInterface*>& plugins);
private:
bool Evaluate(const PluginCleaningData& cleaningData,
const std::string& pluginName);
std::shared_ptr<lci_state> lciState_;
std::unique_ptr<lci_state, decltype(&lci_state_destroy)> lciState_;
};
void ParseCondition(const std::string& condition);
+1
View File
@@ -28,6 +28,7 @@
#include <filesystem>
#include <optional>
#include <string>
#include <unordered_map>
#include <unordered_set>
#include <vector>
+9 -8
View File
@@ -38,11 +38,13 @@ using std::string;
namespace loot {
Plugin::Plugin(const GameType gameType,
std::shared_ptr<GameCache> gameCache,
const GameCache& gameCache,
std::filesystem::path pluginPath,
const bool headerOnly) :
name_(pluginPath.filename().u8string()),
esPlugin(nullptr),
esPlugin(
std::unique_ptr<::Plugin, decltype(&esp_plugin_free)>(nullptr,
esp_plugin_free)),
isEmpty_(true),
loadsArchive_(false),
numOverrideRecords_(0) {
@@ -169,7 +171,7 @@ bool Plugin::LoadsArchive() const { return loadsArchive_; }
bool Plugin::DoFormIDsOverlap(const PluginInterface& plugin) const {
try {
auto otherPlugin = dynamic_cast<const Plugin&>(plugin);
auto& otherPlugin = dynamic_cast<const Plugin&>(plugin);
bool doPluginsOverlap = false;
const auto ret = esp_plugin_do_records_overlap(
@@ -192,8 +194,7 @@ bool Plugin::DoFormIDsOverlap(const PluginInterface& plugin) const {
return false;
}
size_t Plugin::GetOverlapSize(
const std::vector<std::shared_ptr<const Plugin>> plugins) const {
size_t Plugin::GetOverlapSize(const std::vector<const Plugin*> plugins) const {
if (plugins.empty()) {
return 0;
}
@@ -278,7 +279,7 @@ void Plugin::Load(const std::filesystem::path& path,
" : esplugin error code: " + std::to_string(ret));
}
esPlugin = std::shared_ptr<std::remove_pointer<::Plugin>::type>(
esPlugin = std::unique_ptr<::Plugin, decltype(&esp_plugin_free)>(
plugin, esp_plugin_free);
ret = esp_plugin_parse(esPlugin.get(), headerOnly);
@@ -352,7 +353,7 @@ bool equivalent(const std::filesystem::path& path1,
// Get whether the plugin loads an archive (BSA/BA2) or not.
bool Plugin::LoadsArchive(const GameType gameType,
const std::shared_ptr<GameCache> gameCache,
const GameCache& gameCache,
const std::filesystem::path& pluginPath) {
if (gameType == GameType::tes3) {
return false;
@@ -386,7 +387,7 @@ bool Plugin::LoadsArchive(const GameType gameType,
auto basenameLength = pluginPath.stem().native().length();
auto pluginExtension = pluginPath.extension().native();
for (const auto& archivePath : gameCache->GetArchivePaths()) {
for (const auto& archivePath : gameCache.GetArchivePaths()) {
// Need to check if it starts with the given plugin's basename,
// but case insensitively. This is hard to do accurately, so
// instead check if the plugin with the same length basename and
+4 -5
View File
@@ -43,7 +43,7 @@ class GameCache;
class Plugin final : public PluginInterface {
public:
explicit Plugin(const GameType gameType,
std::shared_ptr<GameCache> gameCache,
const GameCache& gameCache,
std::filesystem::path pluginPath,
const bool headerOnly);
@@ -62,8 +62,7 @@ public:
bool IsEmpty() const override;
bool LoadsArchive() const override;
bool DoFormIDsOverlap(const PluginInterface& plugin) const override;
size_t GetOverlapSize(
const std::vector<std::shared_ptr<const Plugin>> plugins) const;
size_t GetOverlapSize(const std::vector<const Plugin*> plugins) const;
// Load ordering functions.
size_t NumOverrideFormIDs() const;
@@ -81,12 +80,12 @@ private:
std::string GetDescription() const;
static bool LoadsArchive(const GameType gameType,
const std::shared_ptr<GameCache> gameCache,
const GameCache& gameCache,
const std::filesystem::path& pluginPath);
static unsigned int GetEspluginGameId(GameType gameType);
const std::string name_;
std::shared_ptr<std::remove_pointer<::Plugin>::type> esPlugin;
std::unique_ptr<::Plugin, decltype(&esp_plugin_free)> esPlugin;
bool isEmpty_; // Does the plugin contain any records other than the TES4
// header?
bool loadsArchive_;
+6 -6
View File
@@ -187,7 +187,7 @@ void PluginGraph::AddPluginVertices(Game& game,
// doesn't strictly need this, there is no guarantee that this
// unspecified behaviour will remain in future compiler updates, so
// implement it generally.
auto loadedPlugins = game.GetCache()->GetPlugins();
auto loadedPlugins = game.GetCache().GetPlugins();
std::sort(loadedPlugins.begin(),
loadedPlugins.end(),
[](const auto& lhs, const auto& rhs) {
@@ -205,10 +205,10 @@ void PluginGraph::AddPluginVertices(Game& game,
for (const auto& plugin : loadedPlugins) {
auto masterlistMetadata =
game.GetDatabase()
->GetPluginMetadata(plugin->GetName(), false, true)
.GetPluginMetadata(plugin->GetName(), false, true)
.value_or(PluginMetadata(plugin->GetName()));
auto userMetadata = game.GetDatabase()
->GetPluginUserMetadata(plugin->GetName(), true)
.GetPluginUserMetadata(plugin->GetName(), true)
.value_or(PluginMetadata(plugin->GetName()));
auto pluginSortingData = PluginSortingData(*plugin,
@@ -232,8 +232,8 @@ void PluginGraph::AddPluginVertices(Game& game,
// Map sets of transitive group dependencies to sets of transitive plugin
// dependencies.
auto groups = GetTransitiveAfterGroups(game.GetDatabase()->GetGroups(false),
game.GetDatabase()->GetUserGroups());
auto groups = GetTransitiveAfterGroups(game.GetDatabase().GetGroups(false),
game.GetDatabase().GetUserGroups());
for (auto& group : groups) {
std::unordered_set<std::string> transitivePlugins;
for (const auto& afterGroup : group.second) {
@@ -382,7 +382,7 @@ void PluginGraph::AddHardcodedPluginEdges(Game& game) {
using std::filesystem::u8path;
auto implicitlyActivePlugins =
game.GetLoadOrderHandler()->GetImplicitlyActivePlugins();
game.GetLoadOrderHandler().GetImplicitlyActivePlugins();
auto logger = getLogger();
std::set<std::string> processedPluginPaths;
+1 -1
View File
@@ -53,7 +53,7 @@ std::vector<std::string> SortPlugins(
graph.AddHardcodedPluginEdges(game);
std::unordered_map<std::string, Group> groups;
for (const auto& group : game.GetDatabase()->GetGroups()) {
for (const auto& group : game.GetDatabase().GetGroups()) {
groups.emplace(group.GetName(), group);
}
graph.AddGroupEdges(groups);
+4 -4
View File
@@ -31,10 +31,10 @@
#include "api/helpers/text.h"
namespace loot {
std::vector<std::shared_ptr<const Plugin>> GetPluginsSubset(
const std::vector<std::shared_ptr<const Plugin>>& plugins,
std::vector<const Plugin*> GetPluginsSubset(
const std::vector<const Plugin*>& plugins,
const std::vector<std::string>& pluginNames) {
std::vector<std::shared_ptr<const Plugin>> pluginsSubset;
std::vector<const Plugin*> pluginsSubset;
for (const auto& pluginName : pluginNames) {
auto pos = std::find_if(plugins.begin(), plugins.end(), [&](auto plugin) {
@@ -55,7 +55,7 @@ PluginSortingData::PluginSortingData(
const PluginMetadata& userMetadata,
const std::vector<std::string>& loadOrder,
const GameType gameType,
const std::vector<std::shared_ptr<const Plugin>>& loadedPlugins) :
const std::vector<const Plugin*>& loadedPlugins) :
plugin_(plugin),
masterlistLoadAfter_(masterlistMetadata.GetLoadAfterFiles()),
userLoadAfter_(userMetadata.GetLoadAfterFiles()),
+6 -7
View File
@@ -31,13 +31,12 @@
namespace loot {
class PluginSortingData {
public:
explicit PluginSortingData(
const Plugin& plugin,
const PluginMetadata& masterlistMetadata,
const PluginMetadata& userMetadata,
const std::vector<std::string>& loadOrder,
const GameType gameType,
const std::vector<std::shared_ptr<const Plugin>>& loadedPlugins);
explicit PluginSortingData(const Plugin& plugin,
const PluginMetadata& masterlistMetadata,
const PluginMetadata& userMetadata,
const std::vector<std::string>& loadOrder,
const GameType gameType,
const std::vector<const Plugin*>& loadedPlugins);
std::string GetName() const;
bool IsMaster() const;
@@ -115,7 +115,7 @@ protected:
masterlist.close();
}
std::shared_ptr<GameInterface> handle_;
std::unique_ptr<GameInterface> handle_;
const std::filesystem::path masterlistPath;
@@ -70,9 +70,11 @@ protected:
std::filesystem::current_path(dataPath.parent_path().parent_path());
}
void TearDown() override { std::filesystem::current_path(originalWorkingDirectory); }
void TearDown() override {
std::filesystem::current_path(originalWorkingDirectory);
}
std::shared_ptr<GameInterface> handle_;
std::unique_ptr<GameInterface> handle_;
const std::filesystem::path gamePathSymlink;
const std::filesystem::path localPathSymlink;
File diff suppressed because it is too large Load Diff

Some files were not shown because too many files have changed in this diff Show More