From e6afa20c6754b424bec2ff283df4667e609bbb16 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 3 Nov 2025 18:53:27 -0500 Subject: [PATCH] Encapsulate and slightly optimize string contexts These steps are done (originally in order, but I squashed it as the end result is still pretty small, and the churn in the code comments was a bit annoying to keep straight). 1. Create proper struct type for string contexts on the heap This will make it easier to change this type in the future. 2. Make `Value::StringWithContext` iterable This make some for loops a lot more terse. 3. Encapsulate `Value::StringWithContext::Context::elems` It turns out the iterators we just exposed are sufficient. 4. Make `StringWithContext::Context` length-prefixed instead Rather than having a null pointer at the end, have a `size_t` at the beginning. This is the exact same size (note that null pointer is longer than null byte) and thus takes no more space! Also, see the new TODO on naming. The thing we already so-named is a builder type for string contexts, not the on-heap type. The `fromBuilder` static method reflects what the names ought to be too. --- src/libexpr/eval-cache.cc | 10 ++-- src/libexpr/eval.cc | 29 +++++----- src/libexpr/include/nix/expr/value.hh | 54 +++++++++++++++++-- src/libexpr/include/nix/expr/value/context.hh | 13 +++++ 4 files changed, 87 insertions(+), 19 deletions(-) diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index de74d2143..0372d6cc1 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -136,17 +136,19 @@ struct AttrDb }); } - AttrId setString(AttrKey key, std::string_view s, const char ** context = nullptr) + AttrId setString(AttrKey key, std::string_view s, const Value::StringWithContext::Context * context = nullptr) { return doSQLite([&]() { auto state(_state->lock()); if (context) { std::string ctx; - for (const char ** p = context; *p; ++p) { - if (p != context) + bool first = true; + for (auto * elem : *context) { + if (!first) ctx.push_back(' '); - ctx.append(*p); + ctx.append(elem); + first = false; } state->insertAttributeWithContext.use()(key.first)(symbols[key.second])(AttrType::String) (s) (ctx) .exec(); diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 7036df957..47880b9c5 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -821,15 +821,18 @@ void Value::mkString(std::string_view s) mkStringNoCopy(makeImmutableString(s)); } -static const char ** encodeContext(const NixStringContext & context) +Value::StringWithContext::Context * Value::StringWithContext::Context::fromBuilder(const NixStringContext & context) { if (!context.empty()) { - size_t n = 0; - auto ctx = (const char **) allocBytes((context.size() + 1) * sizeof(char *)); - for (auto & i : context) { - ctx[n++] = makeImmutableString({i.to_string()}); + auto ctx = (Value::StringWithContext::Context *) allocBytes(sizeof(size_t) + context.size() * sizeof(char *)); + ctx->size = context.size(); + /* Mapping the original iterator to turn references into + pointers is necessary to make sure that enumerate doesn't + accidently copy the elements when it returns tuples by value. + */ + for (auto [n, i] : enumerate(context | std::views::transform([](const auto & r) { return &r; }))) { + ctx->elems[n] = makeImmutableString({i->to_string()}); } - ctx[n] = nullptr; return ctx; } else return nullptr; @@ -837,12 +840,12 @@ static const char ** encodeContext(const NixStringContext & context) void Value::mkString(std::string_view s, const NixStringContext & context) { - mkStringNoCopy(makeImmutableString(s), encodeContext(context)); + mkStringNoCopy(makeImmutableString(s), Value::StringWithContext::Context::fromBuilder(context)); } void Value::mkStringMove(const char * s, const NixStringContext & context) { - mkStringNoCopy(s, encodeContext(context)); + mkStringNoCopy(s, Value::StringWithContext::Context::fromBuilder(context)); } void Value::mkPath(const SourcePath & path) @@ -2287,9 +2290,9 @@ std::string_view EvalState::forceString(Value & v, const PosIdx pos, std::string void copyContext(const Value & v, NixStringContext & context, const ExperimentalFeatureSettings & xpSettings) { - if (v.context()) - for (const char ** p = v.context(); *p; ++p) - context.insert(NixStringContextElem::parse(*p, xpSettings)); + if (auto * ctx = v.context()) + for (auto * elem : *ctx) + context.insert(NixStringContextElem::parse(elem, xpSettings)); } std::string_view EvalState::forceString( @@ -2309,7 +2312,9 @@ std::string_view EvalState::forceStringNoCtx(Value & v, const PosIdx pos, std::s auto s = forceString(v, pos, errorCtx); if (v.context()) { error( - "the string '%1%' is not allowed to refer to a store path (such as '%2%')", v.string_view(), v.context()[0]) + "the string '%1%' is not allowed to refer to a store path (such as '%2%')", + v.string_view(), + *v.context()->begin()) .withTrace(pos, errorCtx) .debugThrow(); } diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index 706a4fe3f..c3a45d4d2 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -220,7 +220,55 @@ struct ValueBase struct StringWithContext { const char * c_str; - const char ** context; // must be in sorted order + + /** + * The type of the context itself. + * + * Currently, it is length-prefixed array of pointers to + * null-terminated strings. The strings are specially formatted + * to represent a flattening of the recursive sum type that is a + * context element. + * + * @See NixStringContext for an more easily understood type, + * that of the "builder" for this data structure. + */ + struct Context + { + using Elem = const char *; + + /** + * Number of items in the array + */ + size_t size; + + private: + /** + * must be in sorted order + */ + Elem elems[/*size*/]; + public: + + const Elem * begin() const + { + return elems; + } + + const Elem * end() const + { + return &elems[size]; + } + + /** + * @note returns a null pointer to more concisely encode the + * empty context + */ + static Context * fromBuilder(const NixStringContext & context); + }; + + /** + * May be null for a string without context. + */ + const Context * context; }; struct Path @@ -991,7 +1039,7 @@ public: setStorage(b); } - void mkStringNoCopy(const char * s, const char ** context = 0) noexcept + void mkStringNoCopy(const char * s, const Value::StringWithContext::Context * context = nullptr) noexcept { setStorage(StringWithContext{.c_str = s, .context = context}); } @@ -1117,7 +1165,7 @@ public: return getStorage().c_str; } - const char ** context() const noexcept + const Value::StringWithContext::Context * context() const noexcept { return getStorage().context; } diff --git a/src/libexpr/include/nix/expr/value/context.hh b/src/libexpr/include/nix/expr/value/context.hh index dcfacbb21..625adc37b 100644 --- a/src/libexpr/include/nix/expr/value/context.hh +++ b/src/libexpr/include/nix/expr/value/context.hh @@ -24,6 +24,14 @@ public: } }; +/** + * @todo This should be reamed to `StringContextBuilderElem`, since: + * + * 1. We use `*Builder` for off-heap temporary data structures + * + * 2. The `Nix*` is totally redundant. (And my mistake from a long time + * ago.) + */ struct NixStringContextElem { /** @@ -77,6 +85,11 @@ struct NixStringContextElem std::string to_string() const; }; +/** + * @todo This should be renamed to `StringContextBuilder`. + * + * @see NixStringContextElem for explanation why. + */ typedef std::set NixStringContext; } // namespace nix