From 2a2cb63b89da3806a831de145a08838d43c3e854 Mon Sep 17 00:00:00 2001 From: Dion Moult Date: Wed, 16 Sep 2026 21:13:51 +1000 Subject: [PATCH] ifcparse: map argument_type to its stored type, size() as size_t Review: argument_type enumerates the members of type_variant_parameter_pack in order, so express that once as argument_storage_type_t (pinned by static_asserts) and let attribute_value::size() on RocksDB go through a single aggregate_size_() helper instead of spelling each vector type out in the switch. size() now returns size_t; its only caller already took size_t. Also build the RocksDB DeleteRange upper bounds as prefix + ('|' + 1) rather than a literal '}', which read as the {id} placeholder notation. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HNrXDmR88wKPCYwGE21SyH --- src/ifcparse/entity_instance_data.cpp | 28 ++++++++++++++++----------- src/ifcparse/file.cpp | 9 ++++----- src/ifcparse/instance_data.h | 21 +++++++++++++++++++- src/ifcparse/rocksdb_set_view.h | 9 ++++----- 4 files changed, 45 insertions(+), 22 deletions(-) diff --git a/src/ifcparse/entity_instance_data.cpp b/src/ifcparse/entity_instance_data.cpp index 35a2337c94..5cf8f1d047 100644 --- a/src/ifcparse/entity_instance_data.cpp +++ b/src/ifcparse/entity_instance_data.cpp @@ -135,6 +135,12 @@ namespace { #endif throw std::logic_error("RocksDB storage is unavailable"); } + + template + inline size_t aggregate_size_(attribute_value::pointer_type array_, uint8_t storage_model_, size_t instance_name_, const ifcopenshell::declaration* entity_or_type, uint8_t index_) + { + return dispatch_get_>(array_, storage_model_, instance_name_, entity_or_type, index_).size(); + } } attribute_value::operator int64_t() const @@ -289,10 +295,10 @@ bool attribute_value::isNull() const return dispatch_has_(array_, storage_model_, instance_name_, entity_or_type_, index_); } -unsigned int attribute_value::size() const +size_t attribute_value::size() const { if (storage_model_ == 0) { - return array_.storage_ptr->apply_visitor(size_visitor{}, index_); + return (size_t)array_.storage_ptr->apply_visitor(size_visitor{}, index_); } #ifdef IFOPSH_WITH_ROCKSDB else { @@ -302,23 +308,23 @@ unsigned int attribute_value::size() const case Argument_AGGREGATE_OF_EMPTY_AGGREGATE: return 0; case Argument_AGGREGATE_OF_INT: - return (unsigned int)dispatch_get_>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + return aggregate_size_(array_, storage_model_, instance_name_, entity_or_type_, index_); case Argument_AGGREGATE_OF_DOUBLE: - return (unsigned int)dispatch_get_>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + return aggregate_size_(array_, storage_model_, instance_name_, entity_or_type_, index_); case Argument_AGGREGATE_OF_STRING: - return (unsigned int)dispatch_get_>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + return aggregate_size_(array_, storage_model_, instance_name_, entity_or_type_, index_); case Argument_AGGREGATE_OF_BINARY: - return (unsigned int)dispatch_get_>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + return aggregate_size_(array_, storage_model_, instance_name_, entity_or_type_, index_); case Argument_AGGREGATE_OF_ENTITY_INSTANCE: - return (unsigned int)((std::vector)*this).size(); + return aggregate_size_(array_, storage_model_, instance_name_, entity_or_type_, index_); case Argument_AGGREGATE_OF_AGGREGATE_OF_INT: - return (unsigned int)dispatch_get_>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + return aggregate_size_(array_, storage_model_, instance_name_, entity_or_type_, index_); case Argument_AGGREGATE_OF_AGGREGATE_OF_DOUBLE: - return (unsigned int)dispatch_get_>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + return aggregate_size_(array_, storage_model_, instance_name_, entity_or_type_, index_); case Argument_AGGREGATE_OF_AGGREGATE_OF_ENTITY_INSTANCE: - return (unsigned int)((std::vector>)*this).size(); + return aggregate_size_(array_, storage_model_, instance_name_, entity_or_type_, index_); default: - return (unsigned int)-1; + return (size_t)-1; } } #endif diff --git a/src/ifcparse/file.cpp b/src/ifcparse/file.cpp index ec06efd103..4356f026e8 100644 --- a/src/ifcparse/file.cpp +++ b/src/ifcparse/file.cpp @@ -215,12 +215,11 @@ void ifcopenshell::impl::rocks_db_file_storage::process_deletion_inverse(const e auto id = inst.id(); { - // Delete every record referencing inst: all keys under v|{id}|. The - // prefix with its last byte incremented is the exclusive upper bound - // ('}' follows '|'), so no iterator is needed to find the range end. + // Delete every record referencing inst: all keys under v||. The + // exclusive upper bound is the same prefix with its separator + // incremented, so no iterator is needed to find the range end. auto prefix = "v|" + std::to_string(id) + "|"; - auto upper_bound = prefix; - upper_bound.back() = '}'; + auto upper_bound = "v|" + std::to_string(id) + std::string(1, '|' + 1); rocksdb::WriteBatch batch; batch.DeleteRange(prefix, upper_bound); diff --git a/src/ifcparse/instance_data.h b/src/ifcparse/instance_data.h index 7d4061d567..ad41e09c11 100644 --- a/src/ifcparse/instance_data.h +++ b/src/ifcparse/instance_data.h @@ -32,6 +32,7 @@ #undef Handle #include +#include #pragma pop_macro("Handle") @@ -226,6 +227,24 @@ struct pack_to_variant_array> { using in_memory_attribute_storage = pack_to_variant_array::type; +// argument_type enumerates the members of type_variant_parameter_pack in +// order, so a member maps back to the type stored for it. +template +struct pack_element; + +template +struct pack_element> { + template + using type = std::tuple_element_t>; +}; + +template +using argument_storage_type_t = typename pack_element::template type; + +static_assert(std::is_same_v, int64_t>, "argument_type must enumerate type_variant_parameter_pack in order"); +static_assert(std::is_same_v, std::vector>, "argument_type must enumerate type_variant_parameter_pack in order"); +static_assert(std::is_same_v, std::vector>>, "argument_type must enumerate type_variant_parameter_pack in order"); + template struct type_encoder_impl; @@ -427,7 +446,7 @@ public: operator enumeration_reference() const; bool isNull() const; - unsigned int size() const; + size_t size() const; ifcopenshell::argument_type type() const; diff --git a/src/ifcparse/rocksdb_set_view.h b/src/ifcparse/rocksdb_set_view.h index 2893b1b53c..0dcab8cf74 100644 --- a/src/ifcparse/rocksdb_set_view.h +++ b/src/ifcparse/rocksdb_set_view.h @@ -197,17 +197,16 @@ public: return iterator(); } - // Removes the element: every key under prefix + key + "|". The prefix - // with its last byte incremented is the exclusive upper bound ('}' - // follows '|'). Returns 1 if the element existed, 0 otherwise. + // Removes the element: every key under prefix + key + "|". The exclusive + // upper bound is the same prefix with its separator incremented. Returns + // 1 if the element existed, 0 otherwise. size_t erase(const key_type& key) { #ifdef IFOPSH_WITH_ROCKSDB if (find(key) == end()) { return 0; } const std::string lower_bound = prefix_ + key_to_string(key) + "|"; - std::string upper_bound = lower_bound; - upper_bound.back() = '}'; + const std::string upper_bound = prefix_ + key_to_string(key) + std::string(1, '|' + 1); rocksdb::WriteBatch batch; batch.DeleteRange(lower_bound, upper_bound); db_->Write(rocksdb::WriteOptions{}, &batch);