From 5aa0695d1287f59eac6c54ddc4e42c7db5472cef Mon Sep 17 00:00:00 2001 From: Julian Eisel Date: Fri, 26 Jul 2024 15:55:03 +0200 Subject: [PATCH] Refactor: Use std::variant instead of union for asset data Rather than a manually managed union, use `std::variant` which is generally safer. E.g. an invalid access will now throw an exception instead of causing undefined behavior (which may or may not crash, or cause a data corruption). Code is also simplified this way. Pull Request: https://projects.blender.org/blender/blender/pulls/125494 --- .../asset_system/AS_asset_representation.hh | 13 ++---- .../asset_system/intern/asset_library.cc | 2 +- .../intern/asset_representation.cc | 41 +++++++------------ 3 files changed, 18 insertions(+), 38 deletions(-) diff --git a/source/blender/asset_system/AS_asset_representation.hh b/source/blender/asset_system/AS_asset_representation.hh index 96192b3f37f..c7b679fafd1 100644 --- a/source/blender/asset_system/AS_asset_representation.hh +++ b/source/blender/asset_system/AS_asset_representation.hh @@ -16,6 +16,7 @@ #include #include #include +#include #include "BLI_string_ref.hh" #include "BLI_utility_mixins.hh" @@ -38,21 +39,13 @@ class AssetRepresentation : NonCopyable, NonMovable { * within the asset library). */ std::string relative_identifier_; - /** - * Indicate if this is a local or external asset, and as such, which of the union members below - * should be used. - */ - const bool is_local_id_ = false; struct ExternalAsset { std::string name; int id_type = 0; std::unique_ptr metadata_ = nullptr; }; - union { - ExternalAsset external_asset_; - ID *local_asset_id_ = nullptr; /* Non-owning. */ - }; + std::variant asset_; friend class AssetLibrary; @@ -70,7 +63,7 @@ class AssetRepresentation : NonCopyable, NonMovable { AssetRepresentation(StringRef relative_asset_path, ID &id, const AssetLibrary &owner_asset_library); - ~AssetRepresentation(); + ~AssetRepresentation() = default; /** * Create a weak reference for this asset that can be written to files, but can break under a diff --git a/source/blender/asset_system/intern/asset_library.cc b/source/blender/asset_system/intern/asset_library.cc index fcc22f52856..a52de7ed2cc 100644 --- a/source/blender/asset_system/intern/asset_library.cc +++ b/source/blender/asset_system/intern/asset_library.cc @@ -232,7 +232,7 @@ void AssetLibrary::remap_ids_and_remove_invalid(const bke::id::IDRemapper &mappi AssetRepresentation &asset = *asset_ptr; BLI_assert(asset.is_local_id()); - const IDRemapperApplyResult result = mappings.apply(&asset.local_asset_id_, + const IDRemapperApplyResult result = mappings.apply(&std::get(asset.asset_), ID_REMAP_APPLY_DEFAULT); /* Entirely remove assets whose ID is unset. We don't want assets with a null ID pointer. */ diff --git a/source/blender/asset_system/intern/asset_representation.cc b/source/blender/asset_system/intern/asset_representation.cc index 56229b5c671..c10cd21fcb9 100644 --- a/source/blender/asset_system/intern/asset_representation.cc +++ b/source/blender/asset_system/intern/asset_representation.cc @@ -27,34 +27,20 @@ AssetRepresentation::AssetRepresentation(StringRef relative_path, const AssetLibrary &owner_asset_library) : owner_asset_library_(owner_asset_library), relative_identifier_(relative_path), - is_local_id_(false), - external_asset_() + asset_(AssetRepresentation::ExternalAsset{name, id_type, std::move(metadata)}) { - external_asset_.name = name; - external_asset_.id_type = id_type; - external_asset_.metadata_ = std::move(metadata); } AssetRepresentation::AssetRepresentation(StringRef relative_path, ID &id, const AssetLibrary &owner_asset_library) - : owner_asset_library_(owner_asset_library), - relative_identifier_(relative_path), - is_local_id_(true), - local_asset_id_(&id) + : owner_asset_library_(owner_asset_library), relative_identifier_(relative_path), asset_(&id) { if (!id.asset_data) { throw std::invalid_argument("Passed ID is not an asset"); } } -AssetRepresentation::~AssetRepresentation() -{ - if (!is_local_id_) { - external_asset_.~ExternalAsset(); - } -} - AssetWeakReference AssetRepresentation::make_weak_reference() const { return AssetWeakReference::make_reference(owner_asset_library_, relative_identifier_); @@ -62,25 +48,26 @@ AssetWeakReference AssetRepresentation::make_weak_reference() const StringRefNull AssetRepresentation::get_name() const { - if (is_local_id_) { - return local_asset_id_->name + 2; + if (const ID *local_id = this->local_id()) { + return local_id->name + 2; } - - return external_asset_.name; + return std::get(asset_).name; } ID_Type AssetRepresentation::get_id_type() const { - if (is_local_id_) { - return GS(local_asset_id_->name); + if (const ID *local_id = this->local_id()) { + return GS(local_id->name); } - - return ID_Type(external_asset_.id_type); + return ID_Type(std::get(asset_).id_type); } AssetMetaData &AssetRepresentation::get_metadata() const { - return is_local_id_ ? *local_asset_id_->asset_data : *external_asset_.metadata_; + if (const ID *local_id = this->local_id()) { + return *local_id->asset_data; + } + return *std::get(asset_).metadata_; } StringRefNull AssetRepresentation::library_relative_identifier() const @@ -130,12 +117,12 @@ bool AssetRepresentation::get_use_relative_path() const ID *AssetRepresentation::local_id() const { - return is_local_id_ ? local_asset_id_ : nullptr; + return this->is_local_id() ? std::get(asset_) : nullptr; } bool AssetRepresentation::is_local_id() const { - return is_local_id_; + return std::holds_alternative(asset_); } const AssetLibrary &AssetRepresentation::owner_asset_library() const