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
This commit is contained in:
Julian Eisel
2024-07-26 15:55:03 +02:00
parent f9810ce99c
commit 5aa0695d12
3 changed files with 18 additions and 38 deletions

View File

@@ -16,6 +16,7 @@
#include <memory>
#include <optional>
#include <string>
#include <variant>
#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<AssetMetaData> metadata_ = nullptr;
};
union {
ExternalAsset external_asset_;
ID *local_asset_id_ = nullptr; /* Non-owning. */
};
std::variant<ExternalAsset, ID *> 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

View File

@@ -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<ID *>(asset.asset_),
ID_REMAP_APPLY_DEFAULT);
/* Entirely remove assets whose ID is unset. We don't want assets with a null ID pointer. */

View File

@@ -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<ExternalAsset>(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<ExternalAsset>(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<ExternalAsset>(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<ID *>(asset_) : nullptr;
}
bool AssetRepresentation::is_local_id() const
{
return is_local_id_;
return std::holds_alternative<ID *>(asset_);
}
const AssetLibrary &AssetRepresentation::owner_asset_library() const