Merge pull request #14470 from NixOS/ctx-type-alias

Encapsulate and slightly optimize string contexts

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)
@@ -2288,9 +2286,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(
@@ -2310,7 +2308,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