From 318eea040f7ebca3719fc802648be7c1016d495c 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. Co-authored-by: Sergei Zimmerman --- src/libexpr/eval-cache.cc | 10 +-- src/libexpr/eval.cc | 32 ++++----- src/libexpr/include/nix/expr/value.hh | 65 ++++++++++++++++++- src/libexpr/include/nix/expr/value/context.hh | 13 ++++ 4 files changed, 97 insertions(+), 23 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..940c6a937 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -36,6 +36,7 @@ #include #include #include +#include #include #include @@ -821,28 +822,25 @@ 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()}); - } - ctx[n] = nullptr; - return ctx; - } else + if (context.empty()) return nullptr; + + auto ctx = new (allocBytes(sizeof(Context) + context.size() * sizeof(value_type))) Context(context.size()); + std::ranges::transform( + context, ctx->elems, [](const NixStringContextElem & elt) { return makeImmutableString(elt.to_string()); }); + return ctx; } 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 +2285,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 +2307,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..0fb36e1b2 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -220,7 +220,66 @@ 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 value_type = const char *; + using size_type = std::size_t; + using iterator = const value_type *; + + Context(size_type size) + : size_(size) + { + } + + private: + /** + * Number of items in the array + */ + size_type size_; + + /** + * @pre must be in sorted order + */ + value_type elems[]; + + public: + iterator begin() const + { + return elems; + } + + iterator end() const + { + return elems + size(); + } + + size_type size() const + { + return size_; + } + + /** + * @return null pointer when context.empty() + */ + static Context * fromBuilder(const NixStringContext & context); + }; + + /** + * May be null for a string without context. + */ + const Context * context; }; struct Path @@ -991,7 +1050,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 +1176,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..054516bc2 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 renamed 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