From 1bdcdb85decf0e62790b76ec05d81fe6e3f55479 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Fri, 12 Jun 2020 14:26:50 -0400 Subject: [PATCH] CResourceStore: Use unique_ptr where applicable Makes the ownership semantics explicit and prevents leaks from occurring a little better. --- src/Core/GameProject/CResourceEntry.cpp | 34 ++++------ src/Core/GameProject/CResourceEntry.h | 26 +++---- src/Core/GameProject/CResourceIterator.h | 15 ++--- src/Core/GameProject/CResourceStore.cpp | 86 +++++++++++------------- src/Core/GameProject/CResourceStore.h | 3 +- 5 files changed, 76 insertions(+), 88 deletions(-) diff --git a/src/Core/GameProject/CResourceEntry.cpp b/src/Core/GameProject/CResourceEntry.cpp index 199371d8..fceb69d4 100644 --- a/src/Core/GameProject/CResourceEntry.cpp +++ b/src/Core/GameProject/CResourceEntry.cpp @@ -11,23 +11,17 @@ #include CResourceEntry::CResourceEntry(CResourceStore *pStore) - : mpResource(nullptr) - , mpTypeInfo(nullptr) - , mpStore(pStore) - , mpDependencies(nullptr) - , mID( CAssetID::InvalidID(pStore->Game()) ) - , mpDirectory(nullptr) - , mMetadataDirty(false) - , mCachedSize(-1) + : mpStore(pStore) + , mID(CAssetID::InvalidID(pStore->Game())) {} // Static constructors -CResourceEntry* CResourceEntry::CreateNewResource(CResourceStore *pStore, const CAssetID& rkID, - const TString& rkDir, const TString& rkName, - EResourceType Type, bool ExistingResource /*= false*/) +std::unique_ptr CResourceEntry::CreateNewResource(CResourceStore *pStore, const CAssetID& rkID, + const TString& rkDir, const TString& rkName, + EResourceType Type, bool ExistingResource) { // Initialize all entry info with the input data. - CResourceEntry *pEntry = new CResourceEntry(pStore); + auto pEntry = std::unique_ptr(new CResourceEntry(pStore)); pEntry->mID = rkID; pEntry->mName = rkName; pEntry->mCachedUppercaseName = rkName.ToUpper(); @@ -37,7 +31,7 @@ CResourceEntry* CResourceEntry::CreateNewResource(CResourceStore *pStore, const pEntry->mpDirectory = pStore->GetVirtualDirectory(rkDir, true); ASSERT(pEntry->mpDirectory); - pEntry->mpDirectory->AddChild("", pEntry); + pEntry->mpDirectory->AddChild("", pEntry.get()); pEntry->mMetadataDirty = true; @@ -45,7 +39,7 @@ CResourceEntry* CResourceEntry::CreateNewResource(CResourceStore *pStore, const // then instantiate the new resource data so it can be saved as soon as possible. if (!ExistingResource) { - pEntry->mpResource = CResourceFactory::CreateResource(pEntry); + pEntry->mpResource = CResourceFactory::CreateResource(pEntry.get()); if (pEntry->mpResource) { @@ -56,30 +50,30 @@ CResourceEntry* CResourceEntry::CreateNewResource(CResourceStore *pStore, const return pEntry; } -CResourceEntry* CResourceEntry::BuildFromArchive(CResourceStore *pStore, IArchive& rArc) +std::unique_ptr CResourceEntry::BuildFromArchive(CResourceStore *pStore, IArchive& rArc) { // Load all entry info from the archive. - CResourceEntry *pEntry = new CResourceEntry(pStore); + auto pEntry = std::unique_ptr(new CResourceEntry(pStore)); pEntry->SerializeEntryInfo(rArc, false); ASSERT(pEntry->mpTypeInfo); ASSERT(pEntry->mpDirectory); return pEntry; } -CResourceEntry* CResourceEntry::BuildFromDirectory(CResourceStore *pStore, CResTypeInfo *pTypeInfo, - const TString& rkDirPath, const TString& rkName) +std::unique_ptr CResourceEntry::BuildFromDirectory(CResourceStore *pStore, CResTypeInfo *pTypeInfo, + const TString& rkDirPath, const TString& rkName) { // Initialize as much entry info as possible from the input data, then load the rest from the metadata file. ASSERT(pTypeInfo); - CResourceEntry *pEntry = new CResourceEntry(pStore); + auto pEntry = std::unique_ptr(new CResourceEntry(pStore)); pEntry->mpTypeInfo = pTypeInfo; pEntry->mName = rkName; pEntry->mCachedUppercaseName = rkName.ToUpper(); pEntry->mpDirectory = pStore->GetVirtualDirectory(rkDirPath, true); ASSERT(pEntry->mpDirectory); - pEntry->mpDirectory->AddChild("", pEntry); + pEntry->mpDirectory->AddChild("", pEntry.get()); // Make sure we're valid, then load the remaining data from the metadata file ASSERT(pEntry->HasCookedVersion() || pEntry->HasRawVersion()); diff --git a/src/Core/GameProject/CResourceEntry.h b/src/Core/GameProject/CResourceEntry.h index a7cd5c16..1d72c6c2 100644 --- a/src/Core/GameProject/CResourceEntry.h +++ b/src/Core/GameProject/CResourceEntry.h @@ -8,10 +8,12 @@ #include #include #include +#include -class CResource; -class CGameProject; class CDependencyTree; +class CGameProject; +class CResource; +class IInputStream; enum class EResEntryFlag { @@ -28,28 +30,28 @@ DECLARE_FLAGS(EResEntryFlag, FResEntryFlags) class CResourceEntry { std::unique_ptr mpResource; - CResTypeInfo *mpTypeInfo; + CResTypeInfo *mpTypeInfo = nullptr; CResourceStore *mpStore; std::unique_ptr mpDependencies; CAssetID mID; - CVirtualDirectory *mpDirectory; + CVirtualDirectory *mpDirectory = nullptr; TString mName; FResEntryFlags mFlags; - mutable bool mMetadataDirty; - mutable uint64 mCachedSize; + mutable bool mMetadataDirty = false; + mutable uint64 mCachedSize = UINT64_MAX; mutable TString mCachedUppercaseName; // This is used to speed up case-insensitive sorting and filtering. // Private constructor explicit CResourceEntry(CResourceStore *pStore); public: - static CResourceEntry* CreateNewResource(CResourceStore *pStore, const CAssetID& rkID, - const TString& rkDir, const TString& rkName, - EResourceType Type, bool ExistingResource = false); - static CResourceEntry* BuildFromArchive(CResourceStore *pStore, IArchive& rArc); - static CResourceEntry* BuildFromDirectory(CResourceStore *pStore, CResTypeInfo *pTypeInfo, - const TString& rkDirPath, const TString& rkName); + static std::unique_ptr CreateNewResource(CResourceStore *pStore, const CAssetID& rkID, + const TString& rkDir, const TString& rkName, + EResourceType Type, bool ExistingResource = false); + static std::unique_ptr BuildFromArchive(CResourceStore *pStore, IArchive& rArc); + static std::unique_ptr BuildFromDirectory(CResourceStore *pStore, CResTypeInfo *pTypeInfo, + const TString& rkDirPath, const TString& rkName); ~CResourceEntry(); bool LoadMetadata(); diff --git a/src/Core/GameProject/CResourceIterator.h b/src/Core/GameProject/CResourceIterator.h index bd831ae9..9c8fb70e 100644 --- a/src/Core/GameProject/CResourceIterator.h +++ b/src/Core/GameProject/CResourceIterator.h @@ -8,15 +8,14 @@ class CResourceIterator { protected: const CResourceStore *mpkStore; - std::map::const_iterator mIter; - CResourceEntry *mpCurEntry; + std::map>::const_iterator mIter; + CResourceEntry *mpCurEntry = nullptr; public: - CResourceIterator(const CResourceStore *pkStore = gpResourceStore) + explicit CResourceIterator(const CResourceStore *pkStore = gpResourceStore) : mpkStore(pkStore) - , mpCurEntry(nullptr) { - mIter = mpkStore->mResourceEntries.begin(); + mIter = mpkStore->mResourceEntries.cbegin(); Next(); } @@ -26,9 +25,9 @@ public: { do { - if (mIter != mpkStore->mResourceEntries.end()) + if (mIter != mpkStore->mResourceEntries.cend()) { - mpCurEntry = mIter->second; + mpCurEntry = mIter->second.get(); ++mIter; } else mpCurEntry = nullptr; @@ -76,7 +75,7 @@ template class TResourceIterator : public CResourceIterator { public: - TResourceIterator(CResourceStore *pStore = gpResourceStore) + explicit TResourceIterator(CResourceStore *pStore = gpResourceStore) : CResourceIterator(pStore) { if (mpCurEntry && mpCurEntry->ResourceType() != ResType) diff --git a/src/Core/GameProject/CResourceStore.cpp b/src/Core/GameProject/CResourceStore.cpp index 16b806d4..db8ae256 100644 --- a/src/Core/GameProject/CResourceStore.cpp +++ b/src/Core/GameProject/CResourceStore.cpp @@ -38,9 +38,6 @@ CResourceStore::~CResourceStore() { CloseProject(); DestroyUnreferencedResources(); - - for (auto It = mResourceEntries.begin(); It != mResourceEntries.end(); It++) - delete It->second; } void RecursiveGetListOfEmptyDirectories(CVirtualDirectory *pDir, TStringList& rOutList) @@ -69,11 +66,9 @@ bool CResourceStore::SerializeDatabaseCache(IArchive& rArc) { // Make sure deleted resources aren't included in the count. // We can't use CResourceIterator because it skips MarkedForDeletion resources. - for (auto Iter = mResourceEntries.begin(); Iter != mResourceEntries.end(); Iter++) + for (const auto& entry : mResourceEntries) { - CResourceEntry* pEntry = Iter->second; - - if (pEntry->IsMarkedForDeletion()) + if (entry.second->IsMarkedForDeletion()) { ResourceCount--; } @@ -88,9 +83,9 @@ bool CResourceStore::SerializeDatabaseCache(IArchive& rArc) { if (rArc.ParamBegin("Resource", 0)) { - CResourceEntry *pEntry = CResourceEntry::BuildFromArchive(this, rArc); - ASSERT( FindEntry(pEntry->ID()) == nullptr ); - mResourceEntries[pEntry->ID()] = pEntry; + auto pEntry = CResourceEntry::BuildFromArchive(this, rArc); + ASSERT(FindEntry(pEntry->ID()) == nullptr); + mResourceEntries.insert_or_assign(pEntry->ID(), std::move(pEntry)); rArc.ParamEnd(); } } @@ -236,13 +231,7 @@ void CResourceStore::CloseProject() } // Delete all entries from old project - auto It = mResourceEntries.begin(); - - while (It != mResourceEntries.end()) - { - delete It->second; - It = mResourceEntries.erase(It); - } + mResourceEntries.clear(); // Clear deleted files from previous runs TString DeletedPath = DeletedResourcePath(); @@ -304,12 +293,12 @@ CResourceEntry* CResourceStore::FindEntry(const CAssetID& rkID) const { auto Found = mResourceEntries.find(rkID); - if (Found != mResourceEntries.end()) + if (Found != mResourceEntries.cend()) { - CResourceEntry* pEntry = Found->second; + auto& pEntry = Found->second; if (!pEntry->IsMarkedForDeletion()) - return pEntry; + return pEntry.get(); } } @@ -346,8 +335,6 @@ void CResourceStore::ClearDatabase() } // Clear out existing resource entries and directories - for (auto Iter = mResourceEntries.begin(); Iter != mResourceEntries.end(); Iter++) - delete Iter->second; mResourceEntries.clear(); delete mpDatabaseRoot; @@ -389,18 +376,19 @@ bool CResourceStore::BuildFromDirectory(bool ShouldGenerateCacheFile) } // Create resource entry - CResourceEntry *pEntry = CResourceEntry::BuildFromDirectory(this, pTypeInfo, DirPath, ResName); + auto pEntry = CResourceEntry::BuildFromDirectory(this, pTypeInfo, DirPath, ResName); // Validate the entry - CAssetID ID = pEntry->ID(); - ASSERT( mResourceEntries.find(ID) == mResourceEntries.end() ); - ASSERT( ID.Length() == CAssetID::GameIDLength(mGame) ); + const CAssetID ID = pEntry->ID(); + ASSERT(mResourceEntries.find(ID) == mResourceEntries.cend()); + ASSERT(ID.Length() == CAssetID::GameIDLength(mGame)); - mResourceEntries[ID] = pEntry; + mResourceEntries.insert_or_assign(ID, std::move(pEntry)); } - else if (FileUtil::IsDirectory(Path)) + { CreateVirtualDirectory(RelPath); + } } // Generate new cache file @@ -448,30 +436,34 @@ CResourceEntry* CResourceStore::CreateNewResource(const CAssetID& rkID, EResourc CResourceEntry *pEntry = FindEntry(rkID); if (pEntry) + { errorf("Attempted to register resource that's already tracked in the database: %s / %s / %s", *rkID.ToString(), *rkDir, *rkName); - + } else { // Validate directory if (IsValidResourcePath(rkDir, rkName)) { - pEntry = CResourceEntry::CreateNewResource(this, rkID, rkDir, rkName, Type, ExistingResource); - mResourceEntries[rkID] = pEntry; + auto res = CResourceEntry::CreateNewResource(this, rkID, rkDir, rkName, Type, ExistingResource); + auto resPtr = res.get(); + + mResourceEntries.insert_or_assign(rkID, std::move(res)); mDatabaseCacheDirty = true; - if (pEntry->IsLoaded()) + if (resPtr->IsLoaded()) { - TrackLoadedResource(pEntry); + TrackLoadedResource(resPtr); } if (!ExistingResource) { - debugf("CREATED NEW RESOURCE: [%s] %s", *rkID.ToString(), *pEntry->CookedAssetPath()); + debugf("CREATED NEW RESOURCE: [%s] %s", *rkID.ToString(), *resPtr->CookedAssetPath()); } + + return resPtr; } - else - errorf("Invalid resource path, failed to register: %s%s", *rkDir, *rkName); + errorf("Invalid resource path, failed to register: %s%s", *rkDir, *rkName); } return pEntry; @@ -479,19 +471,18 @@ CResourceEntry* CResourceStore::CreateNewResource(const CAssetID& rkID, EResourc CResource* CResourceStore::LoadResource(const CAssetID& rkID) { - if (!rkID.IsValid()) return nullptr; + if (!rkID.IsValid()) + return nullptr; CResourceEntry *pEntry = FindEntry(rkID); - - if (pEntry) - return pEntry->Load(); - - else + if (!pEntry) { // Resource doesn't seem to exist warnf("Can't find requested resource with ID \"%s\"", *rkID.ToString()); return nullptr; } + + return pEntry->Load(); } CResource* CResourceStore::LoadResource(const CAssetID& rkID, EResourceType Type) @@ -515,8 +506,7 @@ CResource* CResourceStore::LoadResource(const CAssetID& rkID, EResourceType Type } } - else - return nullptr; + return nullptr; } CResource* CResourceStore::LoadResource(const TString& rkPath) @@ -544,7 +534,7 @@ CResource* CResourceStore::LoadResource(const TString& rkPath) return pEntry->Load(); } - else return nullptr; + return nullptr; } void CResourceStore::TrackLoadedResource(CResourceEntry *pEntry) @@ -573,8 +563,10 @@ void CResourceStore::DestroyUnreferencedResources() It = mLoadedResources.erase(It); NumDeleted++; } - - else It++; + else + { + ++It; + } } } while (NumDeleted > 0); } diff --git a/src/Core/GameProject/CResourceStore.h b/src/Core/GameProject/CResourceStore.h index 1c213e2d..78a16667 100644 --- a/src/Core/GameProject/CResourceStore.h +++ b/src/Core/GameProject/CResourceStore.h @@ -8,6 +8,7 @@ #include #include #include +#include #include class CGameExporter; @@ -30,7 +31,7 @@ class CResourceStore CGameProject *mpProj = nullptr; EGame mGame{EGame::Prime}; CVirtualDirectory *mpDatabaseRoot = nullptr; - std::map mResourceEntries; + std::map> mResourceEntries; std::map mLoadedResources; bool mDatabaseCacheDirty = false;