From 5bc943489372ab5d080022247324921693f550c5 Mon Sep 17 00:00:00 2001 From: Julian Eisel Date: Thu, 29 Feb 2024 17:30:53 +0100 Subject: [PATCH] Refactor: Make asset catalog class members constant Some of these members are not expected to change throughout the lifetime of the class instances, so make them constant and only set them via the constructor. Making them immutable this way helps making clear which data needs extra attention for thread safety. --- source/blender/asset_system/AS_asset_catalog.hh | 7 +++---- .../asset_system/intern/asset_catalog.cc | 17 +++++------------ .../intern/asset_catalog_definition_file.cc | 5 +++++ .../intern/asset_catalog_definition_file.hh | 4 ++-- .../asset_system/tests/asset_catalog_test.cc | 8 ++++---- 5 files changed, 19 insertions(+), 22 deletions(-) diff --git a/source/blender/asset_system/AS_asset_catalog.hh b/source/blender/asset_system/AS_asset_catalog.hh index 335072bb2d4..c2e5602a776 100644 --- a/source/blender/asset_system/AS_asset_catalog.hh +++ b/source/blender/asset_system/AS_asset_catalog.hh @@ -40,17 +40,17 @@ using OwningAssetCatalogMap = Map>; * directory hierarchy). */ class AssetCatalogService { std::unique_ptr catalog_collection_; + /** * Cached catalog tree storage. Lazy-created by #AssetCatalogService::catalog_tree(). */ std::unique_ptr catalog_tree_; std::mutex catalog_tree_mutex_; - CatalogFilePath asset_library_root_; - Vector> undo_snapshots_; Vector> redo_snapshots_; + const CatalogFilePath asset_library_root_; const bool is_read_only_ = false; public: @@ -59,9 +59,8 @@ class AssetCatalogService { struct read_only_tag {}; public: - AssetCatalogService(); + explicit AssetCatalogService(const CatalogFilePath &asset_library_root = {}); explicit AssetCatalogService(read_only_tag); - explicit AssetCatalogService(const CatalogFilePath &asset_library_root); /** * Set tag indicating that some catalog modifications are unsaved, which could diff --git a/source/blender/asset_system/intern/asset_catalog.cc b/source/blender/asset_system/intern/asset_catalog.cc index 6dfa546491a..6a9767dac0f 100644 --- a/source/blender/asset_system/intern/asset_catalog.cc +++ b/source/blender/asset_system/intern/asset_catalog.cc @@ -33,8 +33,9 @@ namespace blender::asset_system { const CatalogFilePath AssetCatalogService::DEFAULT_CATALOG_FILENAME = "blender_assets.cats.txt"; -AssetCatalogService::AssetCatalogService() - : catalog_collection_(std::make_unique()) +AssetCatalogService::AssetCatalogService(const CatalogFilePath &asset_library_root) + : catalog_collection_(std::make_unique()), + asset_library_root_(asset_library_root) { } @@ -43,12 +44,6 @@ AssetCatalogService::AssetCatalogService(read_only_tag) : AssetCatalogService() const_cast(is_read_only_) = true; } -AssetCatalogService::AssetCatalogService(const CatalogFilePath &asset_library_root) - : AssetCatalogService() -{ - asset_library_root_ = asset_library_root; -} - void AssetCatalogService::tag_has_unsaved_changes(AssetCatalog *edited_catalog) { BLI_assert(!is_read_only_); @@ -372,8 +367,7 @@ void AssetCatalogService::load_single_file(const CatalogFilePath &catalog_defini std::unique_ptr AssetCatalogService::parse_catalog_file( const CatalogFilePath &catalog_definition_file_path) { - auto cdf = std::make_unique(); - cdf->file_path = catalog_definition_file_path; + auto cdf = std::make_unique(catalog_definition_file_path); /* TODO(Sybren): this might have to move to a higher level when supporting multiple CDFs. */ Set seen_paths; @@ -556,8 +550,7 @@ CatalogFilePath AssetCatalogService::find_suitable_cdf_path_for_writing( std::unique_ptr AssetCatalogService::construct_cdf_in_memory( const CatalogFilePath &file_path) const { - auto cdf = std::make_unique(); - cdf->file_path = file_path; + auto cdf = std::make_unique(file_path); for (auto &catalog : catalog_collection_->catalogs_.values()) { cdf->add_new(catalog.get()); diff --git a/source/blender/asset_system/intern/asset_catalog_definition_file.cc b/source/blender/asset_system/intern/asset_catalog_definition_file.cc index 9dc2c0efe9e..1ad59166c46 100644 --- a/source/blender/asset_system/intern/asset_catalog_definition_file.cc +++ b/source/blender/asset_system/intern/asset_catalog_definition_file.cc @@ -151,6 +151,11 @@ std::unique_ptr AssetCatalogDefinitionFile::parse_catalog_line(con return std::make_unique(catalog_id, catalog_path.cleanup(), simple_name); } +AssetCatalogDefinitionFile::AssetCatalogDefinitionFile(const CatalogFilePath &file_path) + : file_path(file_path) +{ +} + bool AssetCatalogDefinitionFile::write_to_disk() const { BLI_assert_msg(!this->file_path.empty(), "Writing to CDF requires its file path to be known"); diff --git a/source/blender/asset_system/intern/asset_catalog_definition_file.hh b/source/blender/asset_system/intern/asset_catalog_definition_file.hh index 78e202b5c13..2dfdcb23d0d 100644 --- a/source/blender/asset_system/intern/asset_catalog_definition_file.hh +++ b/source/blender/asset_system/intern/asset_catalog_definition_file.hh @@ -37,10 +37,10 @@ class AssetCatalogDefinitionFile { const static std::string VERSION_MARKER; const static std::string HEADER; - CatalogFilePath file_path; + const CatalogFilePath file_path; public: - AssetCatalogDefinitionFile() = default; + AssetCatalogDefinitionFile(const CatalogFilePath &file_path); /** * Write the catalog definitions to the same file they were read from. diff --git a/source/blender/asset_system/tests/asset_catalog_test.cc b/source/blender/asset_system/tests/asset_catalog_test.cc index c59f28bc7bd..1df91369057 100644 --- a/source/blender/asset_system/tests/asset_catalog_test.cc +++ b/source/blender/asset_system/tests/asset_catalog_test.cc @@ -1116,9 +1116,9 @@ class TestableAssetCatalogCollection : public AssetCatalogCollection { { return catalog_definition_file_.get(); } - AssetCatalogDefinitionFile *allocate_catalog_definition_file() + AssetCatalogDefinitionFile *allocate_catalog_definition_file(StringRef file_path) { - catalog_definition_file_ = std::make_unique(); + catalog_definition_file_ = std::make_unique(file_path); return get_catalog_definition_file(); } }; @@ -1185,8 +1185,8 @@ TEST_F(AssetCatalogTest, cat_collection_deep_copy__nonempty_cdf) catcoll.get_catalogs().add_new(cat2->catalog_id, std::move(cat2)); catcoll.get_deleted_catalogs().add_new(cat3->catalog_id, std::move(cat3)); - AssetCatalogDefinitionFile *cdf = catcoll.allocate_catalog_definition_file(); - cdf->file_path = "path/to/somewhere.cats.txt"; + AssetCatalogDefinitionFile *cdf = catcoll.allocate_catalog_definition_file( + "path/to/somewhere.cats.txt"); cdf->add_new(cat1_ptr); cdf->add_new(cat2_ptr); cdf->add_new(cat3_ptr);