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 <[email protected]>

Author: Ericson2314

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();

src/libexpr/eval.cc

@@ -36,6 +36,7 @@
 #include <sys/time.h>
 #include <fstream>
 #include <functional>
+#include <ranges>
 
 #include <nlohmann/json.hpp>
 #include <boost/container/small_vector.hpp>
@@ -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<EvalError>(
-            "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();
     }

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<StringWithContext>().c_str;
     }
 
-    const char ** context() const noexcept
+    const Value::StringWithContext::Context * context() const noexcept
     {
         return getStorage<StringWithContext>().context;
     }

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<NixStringContextElem> NixStringContext;
 
 } // namespace nix