From fd125202c887463529e52715d442b8fa6400806b Mon Sep 17 00:00:00 2001 From: Johannes Wolf Date: Sun, 20 Jul 2025 14:12:51 +0200 Subject: [PATCH 1/5] string-pool: Introduce a strong typedef for Ids --- CMakeLists.txt | 1 + include/simfil/model/model.h | 6 +- include/simfil/model/nodes.h | 50 +++++++------- include/simfil/model/nodes.impl.h | 8 +-- include/simfil/model/string-handle.h | 99 ++++++++++++++++++++++++++++ include/simfil/model/string-pool.h | 13 ++-- include/simfil/overlay.h | 6 +- src/completion.cpp | 2 +- src/expressions.h | 2 +- src/model/json.cpp | 2 +- src/model/model.cpp | 17 ++--- src/model/nodes.cpp | 27 ++++---- src/model/string-pool.cpp | 14 ++-- src/overlay.cpp | 10 +-- 14 files changed, 182 insertions(+), 75 deletions(-) create mode 100644 include/simfil/model/string-handle.h diff --git a/CMakeLists.txt b/CMakeLists.txt index e70b9735..543223e0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -84,6 +84,7 @@ target_sources(simfil PUBLIC include/simfil/exception-handler.h include/simfil/model/arena.h + include/simfil/model/string-handle.h include/simfil/model/string-pool.h include/simfil/model/model.h include/simfil/model/nodes.h diff --git a/include/simfil/model/model.h b/include/simfil/model/model.h index 39eeab7a..417f405f 100644 --- a/include/simfil/model/model.h +++ b/include/simfil/model/model.h @@ -70,7 +70,7 @@ class Model : public std::enable_shared_from_this * which contains the `strings()` function. The base * implementation returns an unset optional. */ - virtual std::optional lookupStringId(StringId id) const; + virtual std::optional lookupStringId(const StringHandle& id) const; }; /** @@ -149,7 +149,7 @@ class ModelPool : public Model ModelNode::Ptr newValue(int64_t const& value); ModelNode::Ptr newValue(double const& value); ModelNode::Ptr newValue(std::string_view const& value); - ModelNode::Ptr newValue(StringId handle); + ModelNode::Ptr newValue(StringHandle const& handle); /** Node-type-specific resolve-functions */ [[nodiscard]] @@ -168,7 +168,7 @@ class ModelPool : public Model */ virtual void setStrings(std::shared_ptr const& strings); - std::optional lookupStringId(StringId id) const override; + std::optional lookupStringId(const StringHandle& id) const override; /** Serialization */ virtual void write(std::ostream& outputStream); diff --git a/include/simfil/model/nodes.h b/include/simfil/model/nodes.h index 72e34078..f5973313 100644 --- a/include/simfil/model/nodes.h +++ b/include/simfil/model/nodes.h @@ -210,13 +210,13 @@ struct ModelNode [[nodiscard]] virtual ValueType type() const; /// Get a child by name - [[nodiscard]] virtual Ptr get(StringId const& f) const; + [[nodiscard]] virtual Ptr get(StringHandle const& f) const; /// Get a child by index [[nodiscard]] virtual Ptr at(int64_t i) const; /// Get an Object model's field names - [[nodiscard]] virtual StringId keyAt(int64_t i) const; + [[nodiscard]] virtual StringHandle keyAt(int64_t i) const; /// Get the number of children [[nodiscard]] virtual uint32_t size() const; @@ -248,32 +248,32 @@ struct ModelNode /// * Normal `begin()`/`end()`: Supports `for(ModelNode::Ptr child : node)` // Implement fieldNames() by creating an iterator for StringId - class StringIdIterator + class StringHandleIterator { int64_t index; ModelNode const* parent; public: using iterator_category = std::input_iterator_tag; - using value_type = StringId; + using value_type = StringHandle; using difference_type = std::ptrdiff_t; - using pointer = StringId*; - using reference = StringId&; - StringIdIterator(int64_t idx, ModelNode const* p) : index(idx), parent(p) {} - StringIdIterator& operator++() { ++index; return *this; } - bool operator==(const StringIdIterator& other) const { return index == other.index; } - bool operator!=(const StringIdIterator& other) const { return index != other.index; } - StringId operator*() const { return parent->keyAt(index); } + using pointer = StringHandle*; + using reference = StringHandle&; + StringHandleIterator(int64_t idx, ModelNode const* p) : index(idx), parent(p) {} + StringHandleIterator& operator++() { ++index; return *this; } + bool operator==(const StringHandleIterator& other) const { return index == other.index; } + bool operator!=(const StringHandleIterator& other) const { return index != other.index; } + StringHandle operator*() const { return parent->keyAt(index); } }; class StringIdRange { ModelNode const* node; public: explicit StringIdRange(ModelNode const* const n) : node(n) {} - [[nodiscard]] StringIdIterator begin() const { return node->fieldNamesBegin(); } - [[nodiscard]] StringIdIterator end() const { return node->fieldNamesEnd(); } + [[nodiscard]] StringHandleIterator begin() const { return node->fieldNamesBegin(); } + [[nodiscard]] StringHandleIterator end() const { return node->fieldNamesEnd(); } }; - [[nodiscard]] StringIdIterator fieldNamesBegin() const { return {0, this}; } - [[nodiscard]] StringIdIterator fieldNamesEnd() const { return {size(), this}; } + [[nodiscard]] StringHandleIterator fieldNamesBegin() const { return {0, this}; } + [[nodiscard]] StringHandleIterator fieldNamesEnd() const { return {size(), this}; } [[nodiscard]] auto fieldNames() const { return StringIdRange{this}; } // Implement fields() by creating an iterator for field-value pairs @@ -283,7 +283,7 @@ struct ModelNode ModelNode const* parent; public: using iterator_category = std::input_iterator_tag; - using value_type = std::pair; + using value_type = std::pair; using difference_type = std::ptrdiff_t; using pointer = value_type*; using reference = value_type&; @@ -291,7 +291,7 @@ struct ModelNode FieldIterator& operator++() { ++index; return *this; } bool operator==(const FieldIterator& other) const { return index == other.index; } bool operator!=(const FieldIterator& other) const { return index != other.index; } - std::pair operator*() const { return std::make_pair(parent->keyAt(index), parent->at(index)); } + value_type operator*() const { return std::make_pair(parent->keyAt(index), parent->at(index)); } }; class FieldRange { @@ -360,9 +360,9 @@ struct ModelNodeBase : public ModelNode [[nodiscard]] ScalarValueType value() const override; [[nodiscard]] ValueType type() const override; - [[nodiscard]] ModelNode::Ptr get(const StringId&) const override; + [[nodiscard]] ModelNode::Ptr get(const StringHandle&) const override; [[nodiscard]] ModelNode::Ptr at(int64_t) const override; - [[nodiscard]] StringId keyAt(int64_t) const override; + [[nodiscard]] StringHandle keyAt(int64_t) const override; [[nodiscard]] uint32_t size() const override; bool iterate(IterCallback const&) const override {return true;} // NOLINT (allow discard) @@ -525,8 +525,8 @@ struct BaseObject : public MandatoryDerivedModelNodeBase [[nodiscard]] ValueType type() const override; [[nodiscard]] ModelNode::Ptr at(int64_t) const override; [[nodiscard]] uint32_t size() const override; - [[nodiscard]] ModelNode::Ptr get(const StringId &) const override; - [[nodiscard]] StringId keyAt(int64_t) const override; + [[nodiscard]] ModelNode::Ptr get(const StringHandle&) const override; + [[nodiscard]] StringHandle keyAt(int64_t) const override; bool iterate(ModelNode::IterCallback const& cb) const override; // NOLINT (allow discard) protected: @@ -538,6 +538,7 @@ struct BaseObject : public MandatoryDerivedModelNodeBase { Field() = default; Field(StringId name, ModelNodeAddress a) : name_(name), node_(a) {} + Field(StringHandle name, ModelNodeAddress a) : name_(static_cast(name)), node_(a) {} StringId name_ = StringPool::Empty; ModelNodeAddress node_; @@ -579,6 +580,7 @@ struct Object : public BaseObject Object& addField(std::string_view const& name, int64_t const& value); Object& addField(std::string_view const& name, double const& value); Object& addField(std::string_view const& name, std::string_view const& value); + Object& addField(std::string_view const& name, StringHandle const& value); [[nodiscard]] ModelNode::Ptr get(std::string_view const& fieldName) const; @@ -610,16 +612,16 @@ class ProceduralObject : public Object return fields_.size() + (members_ != InvalidArrayIndex ? Object::size() : 0); } - [[nodiscard]] ModelNode::Ptr get(const StringId & field) const override { + [[nodiscard]] ModelNode::Ptr get(const StringHandle& field) const override { for (auto const& [k, v] : fields_) - if (k == field) + if (field == k) return v(static_cast(*this)); if (members_ != InvalidArrayIndex) return Object::get(field); return {}; } - [[nodiscard]] StringId keyAt(int64_t i) const override { + [[nodiscard]] StringHandle keyAt(int64_t i) const override { if (i < fields_.size()) return fields_[i].first; if (members_ != InvalidArrayIndex) diff --git a/include/simfil/model/nodes.impl.h b/include/simfil/model/nodes.impl.h index 775e2dce..04be7abf 100644 --- a/include/simfil/model/nodes.impl.h +++ b/include/simfil/model/nodes.impl.h @@ -100,7 +100,7 @@ ModelNode::Ptr BaseObject::at(int64_t i) const } template -StringId BaseObject::keyAt(int64_t i) const +StringHandle BaseObject::keyAt(int64_t i) const { if (i < 0 || i >= (int64_t)storage_->size(members_)) return {}; @@ -114,14 +114,14 @@ uint32_t BaseObject::size() const } template -ModelNode::Ptr BaseObject::get(const StringId& field) const +ModelNode::Ptr BaseObject::get(const StringHandle& field) const { ModelNode::Ptr result; storage_->iterate( members_, [&field, &result, this](auto&& member) { - if (member.name_ == field) { + if (field == member.name_) { result = ModelNode::Ptr::make(model_, member.node_); return false; } @@ -155,4 +155,4 @@ BaseObject& BaseObject::addF return *this; } -} // namespace simfil \ No newline at end of file +} // namespace simfil diff --git a/include/simfil/model/string-handle.h b/include/simfil/model/string-handle.h new file mode 100644 index 00000000..213775d9 --- /dev/null +++ b/include/simfil/model/string-handle.h @@ -0,0 +1,99 @@ +#pragma once + +#include +#include + +namespace simfil +{ + +namespace detail +{ + +template +struct UnderlyingIntegralType +{ + using Type = Type_; +}; + +template +struct UnderlyingIntegralType>> +{ + using Type = std::underlying_type_t; +}; + +template +using UnderlyingIntegralTypeT = typename UnderlyingIntegralType::Type; + +template +using IsSafeIntegerConversion = + std::integral_constant< + bool, + std::is_integral_v> && + std::is_signed_v> == std::is_signed_v && + sizeof(UnderlyingIntegralTypeT) <= sizeof(To)>; + +template +using EnableSafeIntegerConversion = std::enable_if_t::value>; + +} // namespace detail + +using StringId = std::uint16_t; +static_assert(std::is_unsigned_v, "StringId must be unsigned!"); + +/// StringHandle is a strong typedef for StringId +/// and should be used as a replacement, if possible. +struct StringHandle +{ + using Type = StringId; + + Type value; + + StringHandle() : value(0u) {} + StringHandle(const StringHandle& id) = default; + + template > + constexpr StringHandle(const T& value) : value(static_cast(value)) {} + + explicit operator bool() const { + return value != (Type)0; + } + + explicit operator Type() const { + return value; + } + + auto operator==(const StringHandle& id) const -> bool { + return value == id.value; + } + + template > + auto operator==(const T& v) const -> bool { + return value == v; + } +}; + +} + +namespace std +{ + +template <> +struct hash { + auto operator()(const simfil::StringHandle &id) const noexcept -> std::size_t { + return std::hash()(id.value); + } +}; + +} // namespace std + + +namespace bitsery +{ + +template +void serialize(S& s, simfil::StringHandle& v) +{ + s.value2b(v.value); +} + +} // namespace bitsery diff --git a/include/simfil/model/string-pool.h b/include/simfil/model/string-pool.h index 14412ed4..cc9d0f55 100644 --- a/include/simfil/model/string-pool.h +++ b/include/simfil/model/string-pool.h @@ -12,12 +12,11 @@ #include #include +#include "string-handle.h" + namespace simfil { -using StringId = uint16_t; -static_assert(std::is_unsigned_v, "StringId must be unsigned!"); - /** * Fast and efficient case-insensitive string interner, * used to store object keys. @@ -45,17 +44,17 @@ struct StringPool /// Use this function to lookup a stored string, or insert it /// if it doesn't exist yet. - StringId emplace(std::string_view const& str); + StringHandle emplace(std::string_view const& str); /// Returns the ID of the given string, or `Empty` if /// no such string was ever inserted. - StringId get(std::string_view const& str); + StringHandle get(std::string_view const& str); /// Get the actual string for the given ID, or /// nullopt if the given ID is invalid. /// Virtual to allow defining an inherited StringPool which understands /// additional StaticStringIds. - virtual std::optional resolve(StringId const& id) const; + virtual std::optional resolve(StringHandle const& id) const; /// Get highest stored field id StringId highest() const; @@ -67,7 +66,7 @@ struct StringPool size_t misses() const; /// Add a static key-string mapping - Warning: Not thread-safe. - void addStaticKey(StringId k, std::string const& v); + void addStaticKey(const StringHandle& k, std::string const& v); /// Serialization - write to stream, starting from a specific /// id offset if necessary (for partial serialisation). diff --git a/include/simfil/overlay.h b/include/simfil/overlay.h index 2539550f..fb0d9332 100644 --- a/include/simfil/overlay.h +++ b/include/simfil/overlay.h @@ -24,12 +24,12 @@ class OverlayNode final : public MandatoryDerivedModelNodeBase void; + auto set(StringHandle const& key, Value const& child) -> void; [[nodiscard]] ScalarValueType value() const override; [[nodiscard]] ValueType type() const override; - [[nodiscard]] ModelNode::Ptr get(const StringId& key) const override; + [[nodiscard]] ModelNode::Ptr get(const StringHandle& key) const override; [[nodiscard]] ModelNode::Ptr at(int64_t i) const override; - [[nodiscard]] StringId keyAt(int64_t i) const override; + [[nodiscard]] StringHandle keyAt(int64_t i) const override; [[nodiscard]] uint32_t size() const override; [[nodiscard]] bool iterate(IterCallback const& cb) const override; }; diff --git a/src/completion.cpp b/src/completion.cpp index 68f62d25..3e102adf 100644 --- a/src/completion.cpp +++ b/src/completion.cpp @@ -81,7 +81,7 @@ auto CompletionFieldExpr::ieval(Context ctx, const Value& val, const ResultFn& r if (val.isa(ValueType::Undef)) return res(ctx, val); - for (StringId id : val.node->fieldNames()) { + for (const StringHandle& id : val.node->fieldNames()) { if (comp_->size() >= comp_->limit) return Result::Stop; diff --git a/src/expressions.h b/src/expressions.h index 6bf22e20..bb7f992b 100644 --- a/src/expressions.h +++ b/src/expressions.h @@ -110,7 +110,7 @@ class FieldExpr : public Expr auto toString() const -> std::string override; std::string name_; - StringId nameId_ = {}; + StringHandle nameId_ = {}; size_t hits_ = 0; size_t evaluations_ = 0; diff --git a/src/model/json.cpp b/src/model/json.cpp index 9248fb1e..f53c7e09 100644 --- a/src/model/json.cpp +++ b/src/model/json.cpp @@ -23,7 +23,7 @@ static ModelNode::Ptr build(const json& j, ModelPool & model) case json::value_t::number_integer: return model.newValue(j.get()); case json::value_t::string: - return model.newValue((StringId)model.strings()->emplace(j.get())); + return model.newValue(model.strings()->emplace(j.get())); default: break; } diff --git a/src/model/model.cpp b/src/model/model.cpp index 55344e05..c42b4ece 100644 --- a/src/model/model.cpp +++ b/src/model/model.cpp @@ -114,12 +114,12 @@ std::vector ModelPool::checkForErrors() const return true; }; - auto validatePooledString = [&, this](StringId const& str) + auto validatePooledString = [&, this](StringHandle const& str) { if (!impl_->strings_) return; if (!impl_->strings_->resolve(str)) - errors.push_back(fmt::format("Bad string ID: {}", str)); + errors.push_back(fmt::format("Bad string ID: {}", static_cast(str))); }; std::function validateModelNode = [&](ModelNode::Ptr node) @@ -286,7 +286,7 @@ ModelNode::Ptr Model::newSmallValue(uint16_t value) return ModelNode(shared_from_this(), {UInt16, (uint32_t)value}); } -std::optional Model::lookupStringId(const simfil::StringId) const +std::optional Model::lookupStringId(const simfil::StringHandle&) const { return {}; } @@ -317,8 +317,9 @@ ModelNode::Ptr ModelPool::newValue(std::string_view const& value) return ModelNode(shared_from_this(), {String, (uint32_t)impl_->columns_.strings_.size()-1}); } -ModelNode::Ptr ModelPool::newValue(StringId handle) { - return ModelNode(shared_from_this(), {PooledString, static_cast(handle)}); +ModelNode::Ptr ModelPool::newValue(const StringHandle& handle) +{ + return ModelNode(shared_from_this(), {PooledString, static_cast(handle.value)}); } model_ptr ModelPool::resolveObject(const ModelNode::Ptr& n) const { @@ -353,14 +354,14 @@ void ModelPool::setStrings(std::shared_ptr const& strings) for (auto memberArray : impl_->columns_.objectMemberArrays_) { for (auto& member : memberArray) { if (auto resolvedName = oldStrings->resolve(member.name_)) - member.name_ = strings->emplace(*resolvedName); + member.name_ = static_cast(strings->emplace(*resolvedName)); } } } -std::optional ModelPool::lookupStringId(const simfil::StringId id) const +std::optional ModelPool::lookupStringId(const simfil::StringHandle& id) const { - return impl_->strings_->resolve(id); + return impl_->strings_->resolve(static_cast(id)); } Object::Storage& ModelPool::objectMemberStorage() { diff --git a/src/model/nodes.cpp b/src/model/nodes.cpp index a30a6b23..299a6143 100644 --- a/src/model/nodes.cpp +++ b/src/model/nodes.cpp @@ -29,11 +29,10 @@ ValueType ModelNode::type() const { } /// Get a child by name -ModelNode::Ptr ModelNode::get(const StringId& field) const { +ModelNode::Ptr ModelNode::get(const StringHandle& field) const { ModelNode::Ptr result; if (model_) - model_ - ->resolve(*this, Model::Lambda([&](auto&& resolved) { result = resolved.get(field); })); + model_->resolve(*this, Model::Lambda([&](auto&& resolved) { result = resolved.get(field); })); return result; } @@ -47,10 +46,10 @@ ModelNode::Ptr ModelNode::at(int64_t index) const { } /// Get an Object model's field names -StringId ModelNode::keyAt(int64_t i) const { - StringId result = 0; +StringHandle ModelNode::keyAt(int64_t i) const { + StringHandle result = {}; if (model_) - model_->resolve(*this, Model::Lambda([&](auto&& resolved) { result = resolved.keyAt(i); })); + model_->resolve(*this, Model::Lambda([&](const auto& resolved) { result = resolved.keyAt(i); })); return result; } @@ -58,7 +57,7 @@ StringId ModelNode::keyAt(int64_t i) const { uint32_t ModelNode::size() const { uint32_t result = 0; if (model_) - model_->resolve(*this, Model::Lambda([&](auto&& resolved) { result = resolved.size(); })); + model_->resolve(*this, Model::Lambda([&](const auto& resolved) { result = resolved.size(); })); return result; } @@ -66,7 +65,7 @@ uint32_t ModelNode::size() const { bool ModelNode::iterate(const IterCallback& cb) const { bool result = true; if (model_) - model_->resolve(*this, Model::Lambda([&](auto&& resolved) { result = resolved.iterate(cb); })); + model_->resolve(*this, Model::Lambda([&](const auto& resolved) { result = resolved.iterate(cb); })); return result; } @@ -148,7 +147,7 @@ ValueType ModelNodeBase::type() const return ValueType::Null; } -ModelNode::Ptr ModelNodeBase::get(const StringId&) const +ModelNode::Ptr ModelNodeBase::get(const StringHandle&) const { return nullptr; } @@ -158,9 +157,9 @@ ModelNode::Ptr ModelNodeBase::at(int64_t) const return nullptr; } -StringId ModelNodeBase::keyAt(int64_t) const +StringHandle ModelNodeBase::keyAt(int64_t) const { - return 0; + return {}; } uint32_t ModelNodeBase::size() const @@ -289,6 +288,12 @@ Object& Object::addField(std::string_view const& name, std::string_view const& v return *this; } +Object& Object::addField(std::string_view const& name, StringHandle const& value) { + auto fieldId = model().strings()->emplace(name); + storage_->emplace_back(members_, fieldId, model().newValue(value)->addr()); + return *this; +} + Object& Object::extend(model_ptr const& other) { auto otherSize = other->size(); diff --git a/src/model/string-pool.cpp b/src/model/string-pool.cpp index 8c3a11db..b54b98f3 100644 --- a/src/model/string-pool.cpp +++ b/src/model/string-pool.cpp @@ -89,7 +89,7 @@ StringPool::StringPool(const StringPool& other) cacheMisses_ = other.cacheMisses_.load(); } -StringId StringPool::emplace(std::string_view const& str) +StringHandle StringPool::emplace(std::string_view const& str) { { std::shared_lock lock(stringStoreMutex_); @@ -123,7 +123,7 @@ StringId StringPool::emplace(std::string_view const& str) } } -StringId StringPool::get(std::string_view const& str) +StringHandle StringPool::get(std::string_view const& str) { std::shared_lock stringStoreReadAccess_(stringStoreMutex_); auto it = idForString_.find(str); @@ -134,10 +134,10 @@ StringId StringPool::get(std::string_view const& str) return StringPool::Empty; } -std::optional StringPool::resolve(const StringId& id) const +std::optional StringPool::resolve(const StringHandle& id) const { std::shared_lock stringStoreReadAccess_(stringStoreMutex_); - auto it = stringForId_.find(id); + auto it = stringForId_.find(static_cast(id)); if (it != stringForId_.end()) return it->second; return std::nullopt; @@ -169,12 +169,12 @@ size_t StringPool::misses() const return cacheMisses_; } -void StringPool::addStaticKey(StringId id, const std::string& value) +void StringPool::addStaticKey(const StringHandle& id, const std::string& value) { std::unique_lock lock(stringStoreMutex_); auto& storedString = storedStrings_.emplace_back(value); - idForString_.emplace(storedString, id); - stringForId_.emplace(id, storedString); + idForString_.emplace(storedString, static_cast(id)); + stringForId_.emplace(static_cast(id), storedString); } void StringPool::write(std::ostream& outputStream, const StringId offset) const // NOLINT diff --git a/src/overlay.cpp b/src/overlay.cpp index 612ea8ec..18fea2f8 100644 --- a/src/overlay.cpp +++ b/src/overlay.cpp @@ -18,9 +18,9 @@ OverlayNode::OverlayNode(ModelNode const& n) : MandatoryDerivedModelNodeBase(n) {} -auto OverlayNode::set(StringId const& key, Value const& child) -> void +auto OverlayNode::set(StringHandle const& key, Value const& child) -> void { - model().overlayChildren_.insert({key, child}); + model().overlayChildren_.insert({static_cast(key), child}); } [[nodiscard]] ScalarValueType OverlayNode::value() const @@ -33,9 +33,9 @@ auto OverlayNode::set(StringId const& key, Value const& child) -> void return ValueType::Object; } -[[nodiscard]] ModelNode::Ptr OverlayNode::get(const StringId& key) const +[[nodiscard]] ModelNode::Ptr OverlayNode::get(const StringHandle& key) const { - auto iter = model().overlayChildren_.find(key); + auto iter = model().overlayChildren_.find(static_cast(key)); if (iter != model().overlayChildren_.end()) { if (iter->second.node) return iter->second.node; @@ -49,7 +49,7 @@ auto OverlayNode::set(StringId const& key, Value const& child) -> void return model().value_.node->at(i); } -[[nodiscard]] StringId OverlayNode::keyAt(int64_t i) const +[[nodiscard]] StringHandle OverlayNode::keyAt(int64_t i) const { return model().value_.node->keyAt(i); } From 3d60e4e2a85e742ad0a9ed38f98f6fc430668f4b Mon Sep 17 00:00:00 2001 From: Johannes Wolf Date: Sun, 20 Jul 2025 15:03:14 +0200 Subject: [PATCH 2/5] string-pool: Remove StringId --- include/simfil/model/bitsery-traits.h | 4 +- include/simfil/model/nodes.h | 13 +++-- include/simfil/model/string-handle.h | 71 ++++++++++++++++++++------- include/simfil/model/string-pool.h | 12 ++--- include/simfil/overlay.h | 2 +- src/model/model.cpp | 8 +-- src/model/string-pool.cpp | 33 +++++++------ src/overlay.cpp | 4 +- test/complex.cpp | 4 +- 9 files changed, 93 insertions(+), 58 deletions(-) diff --git a/include/simfil/model/bitsery-traits.h b/include/simfil/model/bitsery-traits.h index 15fe8f71..e81d274e 100644 --- a/include/simfil/model/bitsery-traits.h +++ b/include/simfil/model/bitsery-traits.h @@ -40,9 +40,9 @@ void serialize(S& s, double& v) } template -void serialize(S& s, simfil::StringId& v) +void serialize(S& s, simfil::StringHandle& v) { - s.value2b(v); + s.value2b(v.value); } namespace ext diff --git a/include/simfil/model/nodes.h b/include/simfil/model/nodes.h index f5973313..3da926c6 100644 --- a/include/simfil/model/nodes.h +++ b/include/simfil/model/nodes.h @@ -243,11 +243,11 @@ struct ModelNode virtual bool iterate(IterCallback const& cb) const; // NOLINT (allow discard) /// Iterator Support - /// * `fieldNames()`: Returns range object which supports `for(StringId const& f: node.fieldNames())` + /// * `fieldNames()`: Returns range object which supports `for(StringHandle const& f: node.fieldNames())` /// * `fields()`: Returns range object which supports `for(auto const& [fieldId, value] : node.fields())` /// * Normal `begin()`/`end()`: Supports `for(ModelNode::Ptr child : node)` - // Implement fieldNames() by creating an iterator for StringId + // Implement fieldNames() by creating an iterator for StringHandle class StringHandleIterator { int64_t index; @@ -537,14 +537,13 @@ struct BaseObject : public MandatoryDerivedModelNodeBase struct Field { Field() = default; - Field(StringId name, ModelNodeAddress a) : name_(name), node_(a) {} - Field(StringHandle name, ModelNodeAddress a) : name_(static_cast(name)), node_(a) {} - StringId name_ = StringPool::Empty; + Field(StringHandle name, ModelNodeAddress a) : name_(name), node_(a) {} + StringHandle name_ = StringPool::Empty; ModelNodeAddress node_; template void serialize(S& s) { - s.value2b(name_); + s.object(name_); s.object(node_); } }; @@ -650,7 +649,7 @@ class ProceduralObject : public Object : Object(InvalidArrayIndex, pool, a) {} sfl::small_vector< - std::pair>, + std::pair>, MaxProceduralFields> fields_; }; diff --git a/include/simfil/model/string-handle.h b/include/simfil/model/string-handle.h index 213775d9..75203a8d 100644 --- a/include/simfil/model/string-handle.h +++ b/include/simfil/model/string-handle.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -37,14 +38,9 @@ using EnableSafeIntegerConversion = std::enable_if_t, "StringId must be unsigned!"); - -/// StringHandle is a strong typedef for StringId -/// and should be used as a replacement, if possible. struct StringHandle { - using Type = StringId; + using Type = std::uint16_t; Type value; @@ -54,6 +50,17 @@ struct StringHandle template > constexpr StringHandle(const T& value) : value(static_cast(value)) {} + auto next() const -> StringHandle { + auto n = StringHandle{static_cast(value + 1u)}; + assert(*this < n); + return n; + } + + auto previous() const -> StringHandle { + assert(value > 0); + return {static_cast(value - 1u)}; + } + explicit operator bool() const { return value != (Type)0; } @@ -66,10 +73,50 @@ struct StringHandle return value == id.value; } + auto operator<(const StringHandle& id) const -> bool { + return value < id.value; + } + + auto operator<=(const StringHandle& id) const -> bool { + return value <= id.value; + } + template > auto operator==(const T& v) const -> bool { return value == v; } + + template > + auto operator<(const T& v) const -> bool { + return value < v; + } + + template > + auto operator<=(const T& v) const -> bool { + return value <= v; + } + + template > + auto operator+(const T& v) const -> StringHandle { + return value + v; + } + + /// StringHandle is used as an index type + auto operator++(int) -> StringHandle { + return value++; + } + + auto operator++() -> StringHandle { + return ++value; + } + + auto operator--(int) -> StringHandle { + return value--; + } + + auto operator--() -> StringHandle { + return --value; + } }; } @@ -85,15 +132,3 @@ struct hash { }; } // namespace std - - -namespace bitsery -{ - -template -void serialize(S& s, simfil::StringHandle& v) -{ - s.value2b(v.value); -} - -} // namespace bitsery diff --git a/include/simfil/model/string-pool.h b/include/simfil/model/string-pool.h index cc9d0f55..08192098 100644 --- a/include/simfil/model/string-pool.h +++ b/include/simfil/model/string-pool.h @@ -23,7 +23,7 @@ namespace simfil */ struct StringPool { - enum StaticStringIds : StringId { + enum StaticStringIds : StringHandle::Type { Empty = 0, OverlaySum, OverlayValue, @@ -57,7 +57,7 @@ struct StringPool virtual std::optional resolve(StringHandle const& id) const; /// Get highest stored field id - StringId highest() const; + StringHandle highest() const; /// Get stats size_t size() const; @@ -70,7 +70,7 @@ struct StringPool /// Serialization - write to stream, starting from a specific /// id offset if necessary (for partial serialisation). - virtual void write(std::ostream& outputStream, StringId offset = {}) const; // NOLINT + virtual void write(std::ostream& outputStream, StringHandle offset = {}) const; // NOLINT virtual void read(std::istream& inputStream); /// Check if the content of the string pools is logically identical. @@ -83,11 +83,11 @@ struct StringPool mutable std::shared_mutex stringStoreMutex_; std::unordered_map< std::string_view, - StringId> + StringHandle> idForString_; - std::unordered_map stringForId_; + std::unordered_map stringForId_; std::deque storedStrings_; - StringId nextId_ = FirstDynamicId; + StringHandle nextId_ = FirstDynamicId; std::atomic_int64_t byteSize_{0}; std::atomic_int64_t cacheHits_{0}; std::atomic_int64_t cacheMisses_{0}; diff --git a/include/simfil/overlay.h b/include/simfil/overlay.h index fb0d9332..133c2987 100644 --- a/include/simfil/overlay.h +++ b/include/simfil/overlay.h @@ -10,7 +10,7 @@ namespace simfil struct OverlayNodeStorage final : public Model { Value value_; - std::map overlayChildren_; + std::map overlayChildren_; explicit OverlayNodeStorage(Value const& val) : value_(val) {} // NOLINT diff --git a/src/model/model.cpp b/src/model/model.cpp index c42b4ece..df6f8e45 100644 --- a/src/model/model.cpp +++ b/src/model/model.cpp @@ -142,7 +142,7 @@ std::vector ModelPool::checkForErrors() const validateModelNode(member); } else if (node->addr().column() == PooledString) { - validatePooledString(static_cast(node->addr().index())); + validatePooledString(static_cast(node->addr().index())); } resolve(*node, Lambda([](auto&&) {})); } @@ -233,7 +233,7 @@ void ModelPool::resolve(ModelNode const& n, ResolveFn const& cb) const break; } case PooledString: { - auto str = lookupStringId(static_cast(n.addr().index())); + auto str = lookupStringId(static_cast(n.addr().index())); cb(ValueNode(str.value_or(std::string_view{}), shared_from_this())); break; } @@ -354,14 +354,14 @@ void ModelPool::setStrings(std::shared_ptr const& strings) for (auto memberArray : impl_->columns_.objectMemberArrays_) { for (auto& member : memberArray) { if (auto resolvedName = oldStrings->resolve(member.name_)) - member.name_ = static_cast(strings->emplace(*resolvedName)); + member.name_ = strings->emplace(*resolvedName); } } } std::optional ModelPool::lookupStringId(const simfil::StringHandle& id) const { - return impl_->strings_->resolve(static_cast(id)); + return impl_->strings_->resolve(id); } Object::Storage& ModelPool::objectMemberStorage() { diff --git a/src/model/string-pool.cpp b/src/model/string-pool.cpp index b54b98f3..6e62c2bd 100644 --- a/src/model/string-pool.cpp +++ b/src/model/string-pool.cpp @@ -1,5 +1,6 @@ #include "simfil/model/string-pool.h" #include "simfil/exception-handler.h" +#include "simfil/model/bitsery-traits.h" #include #include @@ -110,7 +111,7 @@ StringHandle StringPool::emplace(std::string_view const& str) // Store the string to maintain ownership. auto& storedString = storedStrings_.emplace_back(str); - StringId id = nextId_++; + StringHandle id = nextId_++; if (nextId_ < id) { raise("StringPool id overflow!"); } @@ -137,15 +138,15 @@ StringHandle StringPool::get(std::string_view const& str) std::optional StringPool::resolve(const StringHandle& id) const { std::shared_lock stringStoreReadAccess_(stringStoreMutex_); - auto it = stringForId_.find(static_cast(id)); + auto it = stringForId_.find(id); if (it != stringForId_.end()) return it->second; return std::nullopt; } -StringId StringPool::highest() const +StringHandle StringPool::highest() const { - return nextId_ - 1; + return nextId_.previous(); } size_t StringPool::size() const @@ -173,30 +174,30 @@ void StringPool::addStaticKey(const StringHandle& id, const std::string& value) { std::unique_lock lock(stringStoreMutex_); auto& storedString = storedStrings_.emplace_back(value); - idForString_.emplace(storedString, static_cast(id)); - stringForId_.emplace(static_cast(id), storedString); + idForString_.emplace(storedString, id); + stringForId_.emplace(id, storedString); } -void StringPool::write(std::ostream& outputStream, const StringId offset) const // NOLINT +void StringPool::write(std::ostream& outputStream, const StringHandle offset) const // NOLINT { std::shared_lock stringStoreReadAccess(stringStoreMutex_); bitsery::Serializer s(outputStream); // Calculate how many strings will be sent - StringId sendStrCount = 0; + StringHandle sendStrCount = {}; const auto high = highest(); for (auto strId = offset; strId <= high; ++strId) { auto it = stringForId_.find(strId); if (it != stringForId_.end()) ++sendStrCount; } - s.value2b(sendStrCount); + s.object(sendStrCount); // Send the pool's key-string pairs for (auto strId = offset; strId <= high; ++strId) { auto it = stringForId_.find(strId); if (it != stringForId_.end()) { - s.value2b(strId); + s.object(strId); // Don't support strings longer than 64kB. s.text1b(it->second, std::numeric_limits::max()); } @@ -209,14 +210,14 @@ void StringPool::read(std::istream& inputStream) bitsery::Deserializer s(inputStream); // Determine how many strings are to be received - StringId rcvStringCount{}; - s.value2b(rcvStringCount); + StringHandle rcvStringCount{}; + s.object(rcvStringCount); // Read strings - for (auto i = 0; i < rcvStringCount; ++i) { + for (auto i = 0u; i < static_cast(rcvStringCount); ++i) { // Read string key - StringId stringId{}; - s.value2b(stringId); + StringHandle stringId{}; + s.object(stringId); // Don't support strings longer than 64kB. auto& stringValue = storedStrings_.emplace_back(); @@ -227,7 +228,7 @@ void StringPool::read(std::istream& inputStream) if (insertionTookPlace) { stringForId_.try_emplace(stringId, stringValue); byteSize_ += static_cast(stringValue.size()); - nextId_ = std::max(nextId_, stringId + 1); + nextId_ = std::max(nextId_, stringId.next()); } } diff --git a/src/overlay.cpp b/src/overlay.cpp index 18fea2f8..27691c16 100644 --- a/src/overlay.cpp +++ b/src/overlay.cpp @@ -20,7 +20,7 @@ OverlayNode::OverlayNode(ModelNode const& n) auto OverlayNode::set(StringHandle const& key, Value const& child) -> void { - model().overlayChildren_.insert({static_cast(key), child}); + model().overlayChildren_.insert({key, child}); } [[nodiscard]] ScalarValueType OverlayNode::value() const @@ -35,7 +35,7 @@ auto OverlayNode::set(StringHandle const& key, Value const& child) -> void [[nodiscard]] ModelNode::Ptr OverlayNode::get(const StringHandle& key) const { - auto iter = model().overlayChildren_.find(static_cast(key)); + auto iter = model().overlayChildren_.find(key); if (iter != model().overlayChildren_.end()) { if (iter->second.node) return iter->second.node; diff --git a/test/complex.cpp b/test/complex.cpp index 0362cdd3..31d24b80 100644 --- a/test/complex.cpp +++ b/test/complex.cpp @@ -149,7 +149,7 @@ TEST_CASE("Serialization", "[complex.serialization]") { REQUIRE(model->strings()->size() == recoveredFields->size()); REQUIRE(model->strings()->highest() == recoveredFields->highest()); REQUIRE(model->strings()->bytes() == recoveredFields->bytes()); - for (StringId sId = 0; sId <= recoveredFields->highest(); ++sId) - REQUIRE(model->strings()->resolve(sId) == recoveredFields->resolve(StringId(sId))); + for (StringHandle sId = {}; sId <= recoveredFields->highest(); ++sId) + REQUIRE(model->strings()->resolve(sId) == recoveredFields->resolve(sId)); } } From c7f3ded34a5545646a5fb43650dca6da784b84e6 Mon Sep 17 00:00:00 2001 From: Johannes Wolf Date: Sun, 20 Jul 2025 15:12:46 +0200 Subject: [PATCH 3/5] string-handle: Fix increment/decrement operators --- include/simfil/model/string-handle.h | 35 ++++++++++++++++------------ 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/include/simfil/model/string-handle.h b/include/simfil/model/string-handle.h index 75203a8d..61830cb3 100644 --- a/include/simfil/model/string-handle.h +++ b/include/simfil/model/string-handle.h @@ -2,6 +2,7 @@ #include #include +#include #include namespace simfil @@ -51,14 +52,13 @@ struct StringHandle constexpr StringHandle(const T& value) : value(static_cast(value)) {} auto next() const -> StringHandle { - auto n = StringHandle{static_cast(value + 1u)}; - assert(*this < n); - return n; + assert(value < std::numeric_limits::max()); + return StringHandle{static_cast(value + 1u)}; } auto previous() const -> StringHandle { assert(value > 0); - return {static_cast(value - 1u)}; + return StringHandle{static_cast(value - 1u)}; } explicit operator bool() const { @@ -96,26 +96,31 @@ struct StringHandle return value <= v; } - template > - auto operator+(const T& v) const -> StringHandle { - return value + v; - } - /// StringHandle is used as an index type auto operator++(int) -> StringHandle { - return value++; + assert(value < std::numeric_limits::max()); + auto old = *this; + ++value; + return old; } - auto operator++() -> StringHandle { - return ++value; + auto operator++() -> StringHandle& { + assert(value < std::numeric_limits::max()); + ++value; + return *this; } auto operator--(int) -> StringHandle { - return value--; + assert(value > 0); + auto old = *this; + --value; + return old; } - auto operator--() -> StringHandle { - return --value; + auto operator--() -> StringHandle& { + assert(value > 0); + --value; + return *this; } }; From 40dca382c98998c4300fa80878576028e4921e7d Mon Sep 17 00:00:00 2001 From: Johannes Wolf Date: Sun, 20 Jul 2025 15:18:58 +0200 Subject: [PATCH 4/5] model: Replace wrong forwarding refs --- src/model/nodes.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/model/nodes.cpp b/src/model/nodes.cpp index 299a6143..aca8c0ce 100644 --- a/src/model/nodes.cpp +++ b/src/model/nodes.cpp @@ -16,7 +16,7 @@ ModelNode::ModelNode(ModelConstPtr pool, ModelNodeAddress addr, ScalarValueType ScalarValueType ModelNode::value() const { ScalarValueType result; if (model_) - model_->resolve(*this, Model::Lambda([&](auto&& resolved) { result = resolved.value(); })); + model_->resolve(*this, Model::Lambda([&](const auto& resolved) { result = resolved.value(); })); return result; } @@ -24,7 +24,7 @@ ScalarValueType ModelNode::value() const { ValueType ModelNode::type() const { ValueType result = ValueType::Null; if (model_) - model_->resolve(*this, Model::Lambda([&](auto&& resolved) { result = resolved.type(); })); + model_->resolve(*this, Model::Lambda([&](const auto& resolved) { result = resolved.type(); })); return result; } @@ -32,7 +32,7 @@ ValueType ModelNode::type() const { ModelNode::Ptr ModelNode::get(const StringHandle& field) const { ModelNode::Ptr result; if (model_) - model_->resolve(*this, Model::Lambda([&](auto&& resolved) { result = resolved.get(field); })); + model_->resolve(*this, Model::Lambda([&](const auto& resolved) { result = resolved.get(field); })); return result; } @@ -41,7 +41,7 @@ ModelNode::Ptr ModelNode::at(int64_t index) const { ModelNode::Ptr result; if (model_) model_ - ->resolve(*this, Model::Lambda([&](auto&& resolved) { result = resolved.at(index); })); + ->resolve(*this, Model::Lambda([&](const auto& resolved) { result = resolved.at(index); })); return result; } From 127198861e0762e2121a066ca62406bd7d6c3555 Mon Sep 17 00:00:00 2001 From: Johannes Wolf Date: Sun, 20 Jul 2025 15:22:08 +0200 Subject: [PATCH 5/5] string-handle: Remove Overflow Checks to Restore Old Behaviour --- include/simfil/model/string-handle.h | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/include/simfil/model/string-handle.h b/include/simfil/model/string-handle.h index 61830cb3..2d353e01 100644 --- a/include/simfil/model/string-handle.h +++ b/include/simfil/model/string-handle.h @@ -39,6 +39,9 @@ using EnableSafeIntegerConversion = std::enable_if_t(value)) {} auto next() const -> StringHandle { - assert(value < std::numeric_limits::max()); return StringHandle{static_cast(value + 1u)}; } auto previous() const -> StringHandle { - assert(value > 0); return StringHandle{static_cast(value - 1u)}; } @@ -98,27 +99,23 @@ struct StringHandle /// StringHandle is used as an index type auto operator++(int) -> StringHandle { - assert(value < std::numeric_limits::max()); auto old = *this; ++value; return old; } auto operator++() -> StringHandle& { - assert(value < std::numeric_limits::max()); ++value; return *this; } auto operator--(int) -> StringHandle { - assert(value > 0); auto old = *this; --value; return old; } auto operator--() -> StringHandle& { - assert(value > 0); --value; return *this; }