diff --git a/src/ifcparse/instance_data.h b/src/ifcparse/instance_data.h index 7d4061d567..b40ad3197f 100644 --- a/src/ifcparse/instance_data.h +++ b/src/ifcparse/instance_data.h @@ -42,6 +42,7 @@ #include #include +#include #include #include @@ -509,8 +510,8 @@ class IFC_PARSE_API instance_data { void populate_derived_(); public: - // Since rocks_db_attribute_storage has no members this is not a variant but in_memory*, where nullptr means a rocks_db_attribute_storage is constructed on the fly given the context from instance data. - in_memory_attribute_storage* storage_; + // Since rocks_db_attribute_storage has no members this is not a variant but an optional in_memory storage, where an empty optional means a rocks_db_attribute_storage is constructed on the fly given the context from instance data. + std::optional storage_; const ifcopenshell::declaration* declaration() const { return declaration_; @@ -529,13 +530,13 @@ class IFC_PARSE_API instance_data { } instance_data(ifcopenshell::file* file, const ifcopenshell::declaration* declaration, uint32_t id, in_memory_attribute_storage&& storage) - : file_(file), declaration_(declaration), identity_(counter_++), id_(id), storage_(new in_memory_attribute_storage(std::move(storage))) + : file_(file), declaration_(declaration), identity_(counter_++), id_(id), storage_(std::move(storage)) { populate_derived_(); } instance_data(ifcopenshell::file* file, const ifcopenshell::declaration* declaration, uint32_t id, rocks_db_attribute_storage&& storage) - : file_(file), declaration_(declaration), identity_(counter_++), id_(id), storage_(nullptr) + : file_(file), declaration_(declaration), identity_(counter_++), id_(id), storage_(std::nullopt) { static_cast(storage); populate_derived_(); @@ -544,7 +545,7 @@ class IFC_PARSE_API instance_data { /* // now that there are referenced as shared_ptr there is no move constructor anymore instance_data(instance_data&& other) noexcept - : file_(other.file_), id_(other.id_), declaration_(other.declaration_), storage_(std::exchange(other.storage_, nullptr)) + : file_(other.file_), id_(other.id_), declaration_(other.declaration_), storage_(std::move(other.storage_)) {} */ @@ -558,15 +559,15 @@ class IFC_PARSE_API instance_data { // same instance_data& operator=(instance_data&& other) noexcept { if (this != &other) { - delete storage_; - storage_ = std::exchange(other.storage_, nullptr); + storage_ = std::move(other.storage_); + other.storage_.reset(); } return *this; } */ ~instance_data() { - delete storage_; + storage_.reset(); } attribute_value get_attribute_value(size_t attribute_index) const; diff --git a/src/ifcparse/parse.cpp b/src/ifcparse/parse.cpp index b5146dfc30..4c29cdd2ea 100644 --- a/src/ifcparse/parse.cpp +++ b/src/ifcparse/parse.cpp @@ -3599,7 +3599,7 @@ instance_data::instance_data(const instance_data& data) attribute_value instance_data::get_attribute_value(size_t index) const { if (storage_) { - return attribute_value(storage_, (uint8_t)index); + return attribute_value(&*storage_, (uint8_t)index); } else { auto* const storage = std::visit([](auto& m) -> ifcopenshell::impl::rocks_db_file_storage* { using U = std::decay_t; diff --git a/src/ifcparse/variant_array.h b/src/ifcparse/variant_array.h index ac4ec34549..6dd2797485 100644 --- a/src/ifcparse/variant_array.h +++ b/src/ifcparse/variant_array.h @@ -140,9 +140,10 @@ class variant_array { public: using types_tuple = ::impl::mapped_types; + // The size byte, the per-slot type indices and the slots live in one + // allocation: [size][index * size][padding][slot * size]. variant_array(size_t size) - : size_and_indices_(size ? new uint8_t[size + 1] : nullptr) - , storage_(size ? new storage_type[size] : nullptr) + : size_and_indices_(size ? new uint8_t[block_bytes_(size)] : nullptr) { if (size) { size_and_indices_[0] = (uint8_t)size; @@ -156,10 +157,8 @@ public: variant_array(variant_array&& other) noexcept : size_and_indices_(other.size_and_indices_) - , storage_(other.storage_) { other.size_and_indices_ = nullptr; - other.storage_ = nullptr; } variant_array& operator=(variant_array&& other) noexcept { @@ -167,10 +166,7 @@ public: free_(); size_and_indices_ = other.size_and_indices_; - storage_ = other.storage_; - other.size_and_indices_ = nullptr; - other.storage_ = nullptr; } return *this; } @@ -192,9 +188,9 @@ public: size_and_indices_[index + 1] = ::impl::TypeIndex_v; using v = typename std::tuple_element<::impl::TypeIndex_v, ::impl::mapped_types>::type; if constexpr (::impl::is_unique_ptr::value) { - new(&storage_[index]) v(new u(value)); + new(&slots_()[index]) v(new u(value)); } else { - new(&storage_[index]) u(std::forward(value)); + new(&slots_()[index]) u(std::forward(value)); } } @@ -223,9 +219,9 @@ public: } using v = typename std::tuple_element<::impl::TypeIndex_v, ::impl::mapped_types>::type; if constexpr (::impl::is_unique_ptr::value) { - return **reinterpret_cast(&storage_[index]); + return **reinterpret_cast(&slots_()[index]); } else { - return *reinterpret_cast(&storage_[index]); + return *reinterpret_cast(&slots_()[index]); } } @@ -251,9 +247,9 @@ public: } using v = typename std::tuple_element<::impl::TypeIndex_v, ::impl::mapped_types>::type; if constexpr (::impl::is_unique_ptr::value) { - return **reinterpret_cast(&storage_[index]); + return **reinterpret_cast(&slots_()[index]); } else { - return *reinterpret_cast(&storage_[index]); + return *reinterpret_cast(&slots_()[index]); } } @@ -273,9 +269,20 @@ public: private: using storage_type = typename ::impl::make_union_from_tuple<::impl::mapped_types>::type; - + static_assert(alignof(storage_type) <= __STDCPP_DEFAULT_NEW_ALIGNMENT__, "slots must fit the alignment new[] guarantees"); uint8_t* size_and_indices_; - storage_type* storage_; + + // The slots follow the index bytes in the same block. + storage_type* slots_() const { + return reinterpret_cast(size_and_indices_ + slots_offset_(size_and_indices_[0])); + } + + static constexpr size_t slots_offset_(size_t size) { + return (size + 1 + alignof(storage_type) - 1) / alignof(storage_type) * alignof(storage_type); + } + static constexpr size_t block_bytes_(size_t size) { + return slots_offset_(size) + size * sizeof(storage_type); + } void destroy_at_index(std::size_t index) { destroy_type_at_index(index, std::integral_constant{}); @@ -287,7 +294,6 @@ private: destroy_at_index(i); } delete[] size_and_indices_; - delete[] storage_; } } @@ -296,7 +302,7 @@ private: if (size_and_indices_[index + 1] == Index - 1) { using t = typename std::tuple_element_t>; if constexpr (!std::is_trivially_destructible::value) { - reinterpret_cast(&storage_[index])->~t(); + reinterpret_cast(&slots_()[index])->~t(); } size_and_indices_[index + 1] = sizeof...(Types); } else { @@ -313,9 +319,9 @@ private: if (size_and_indices_[index + 1] == Index - 1) { using t = typename std::tuple_element_t>; if constexpr (::impl::is_unique_ptr::value) { - return visitor(**reinterpret_cast(&storage_[index])); + return visitor(**reinterpret_cast(&slots_()[index])); } else { - return visitor(*reinterpret_cast(&storage_[index])); + return visitor(*reinterpret_cast(&slots_()[index])); } } return apply_visitor_impl(std::forward(visitor), index, std::integral_constant{});