Merge e6afa20c67 into 479b6b73a9

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

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