Pascal Strings, Baby

Author: glittershark

src/libexpr-c/nix_api_value.cc

@@ -235,7 +235,7 @@ nix_get_string(nix_c_context * context, const nix_value * value, nix_get_string_
     try {
         auto & v = check_value_in(value);
         assert(v.type() == nix::nString);
-        call_nix_get_string_callback(v.c_str(), callback, user_data);
+        call_nix_get_string_callback(v.string_view(), callback, user_data);
     }
     NIXC_CATCH_ERRS
 }
@@ -254,7 +254,7 @@ const char * nix_get_path_string(nix_c_context * context, const nix_value * valu
         // We could use v.path().to_string().c_str(), but I'm concerned this
         // crashes. Looks like .path() allocates a CanonPath with a copy of the
         // string, then it gets the underlying data from that.
-        return v.pathStr();
+        return v.pathStr()->view().data();
     }
     NIXC_CATCH_ERRS_NULL
 }
@@ -439,7 +439,7 @@ nix_get_attr_byidx(nix_c_context * context, nix_value * value, EvalState * state
             return nullptr;
         }
         const nix::Attr & a = (*v.attrs())[i];
-        *name = state->state.symbols[a.name].c_str();
+        *name = state->state.symbols[a.name].string_data()->view().data();
         nix_gc_incref(nullptr, a.value);
         state->state.forceValue(*a.value, nix::noPos);
         return as_nix_value_ptr(a.value);
@@ -460,7 +460,7 @@ nix_value * nix_get_attr_byidx_lazy(
             return nullptr;
         }
         const nix::Attr & a = (*v.attrs())[i];
-        *name = state->state.symbols[a.name].c_str();
+        *name = state->state.symbols[a.name].string_data()->view().data();
         nix_gc_incref(nullptr, a.value);
         // Note: intentionally NOT calling forceValue() to keep the attribute lazy
         return as_nix_value_ptr(a.value);
@@ -480,7 +480,7 @@ const char * nix_get_attr_name_byidx(nix_c_context * context, nix_value * value,
             return nullptr;
         }
         const nix::Attr & a = (*v.attrs())[i];
-        return state->state.symbols[a.name].c_str();
+        return state->state.symbols[a.name].string_data()->view().data();
     }
     NIXC_CATCH_ERRS_NULL
 }

src/libexpr-test-support/include/nix/expr/tests/libexpr.hh

@@ -104,9 +104,11 @@ MATCHER(IsAttrs, "")
 MATCHER_P(IsStringEq, s, fmt("The string is equal to \"%1%\"", s))
 {
     if (arg.type() != nString) {
+        *result_listener << "of non-String type " << arg.type();
         return false;
     }
-    return std::string_view(arg.c_str()) == s;
+    *result_listener << "string(" << arg.string_view() << ")";
+    return arg.string_view() == s;
 }
 
 MATCHER_P(IsIntEq, v, fmt("The string is equal to \"%1%\"", v))

src/libexpr-tests/value/value.cc

@@ -1,6 +1,7 @@
 #include "nix/expr/value.hh"
 
 #include "nix/store/tests/libstore.hh"
+#include <gtest/gtest.h>
 
 namespace nix {
 
@@ -22,4 +23,21 @@ TEST_F(ValueTest, vInt)
     ASSERT_EQ(true, vInt.isValid());
 }
 
+TEST_F(ValueTest, staticString)
+{
+    Value vStr1;
+    Value vStr2;
+    vStr1.mkStringNoCopy<"foo">();
+    vStr2.mkStringNoCopy<"foo">();
+
+    auto sd1 = vStr1.string_data();
+    auto sd2 = vStr2.string_data();
+
+    // The strings should be the same
+    ASSERT_EQ(sd1->view(), sd2->view());
+
+    // The strings should also be backed by the same (static) allocation
+    ASSERT_EQ(std::bit_cast<size_t>(sd1), std::bit_cast<size_t>(sd2));
+}
+
 } // namespace nix

src/libexpr/eval-cache.cc

@@ -406,7 +406,7 @@ Value & AttrCursor::forceValue()
 
     if (root->db && (!cachedValue || std::get_if<placeholder_t>(&cachedValue->second))) {
         if (v.type() == nString)
-            cachedValue = {root->db->setString(getKey(), v.c_str(), v.context()), string_t{v.c_str(), {}}};
+            cachedValue = {root->db->setString(getKey(), v.string_view(), v.context()), string_t{v.string_view(), {}}};
         else if (v.type() == nPath) {
             auto path = v.path().path;
             cachedValue = {root->db->setString(getKey(), path.abs()), string_t{path.abs(), {}}};
@@ -541,7 +541,7 @@ std::string AttrCursor::getString()
     if (v.type() != nString && v.type() != nPath)
         root->state.error<TypeError>("'%s' is not a string but %s", getAttrPathStr(), showType(v)).debugThrow();
 
-    return v.type() == nString ? v.c_str() : v.path().to_string();
+    return v.type() == nString ? std::string(v.string_view()) : v.path().to_string();
 }
 
 string_t AttrCursor::getStringWithContext()
@@ -580,7 +580,7 @@ string_t AttrCursor::getStringWithContext()
     if (v.type() == nString) {
         NixStringContext context;
         copyContext(v, context);
-        return {v.c_str(), std::move(context)};
+        return {std::string(v.string_view()), std::move(context)};
     } else if (v.type() == nPath)
         return {v.path().to_string(), {}};
     else

src/libexpr/eval.cc

@@ -3,6 +3,7 @@
 #include "nix/expr/primops.hh"
 #include "nix/expr/print-options.hh"
 #include "nix/expr/symbol-table.hh"
+#include "nix/expr/value.hh"
 #include "nix/util/exit.hh"
 #include "nix/util/types.hh"
 #include "nix/util/util.hh"
@@ -28,6 +29,8 @@
 #include "parser-tab.hh"
 
 #include <algorithm>
+#include <cstddef>
+#include <cstdlib>
 #include <iostream>
 #include <sstream>
 #include <cstring>
@@ -47,28 +50,22 @@ using json = nlohmann::json;
 
 namespace nix {
 
-static char * allocString(size_t size)
+StringData * StringData::alloc(size_t size)
 {
-    char * t;
-    t = (char *) GC_MALLOC_ATOMIC(size);
-    if (!t)
+    auto res = (StringData *) GC_MALLOC_ATOMIC(sizeof(size_t) + size + 1);
+    if (!res) {
         throw std::bad_alloc();
-    return t;
+    }
+    res->m_size = size;
+    return res;
 }
 
-// When there's no need to write to the string, we can optimize away empty
-// string allocations.
-// This function handles makeImmutableString(std::string_view()) by returning
-// the empty string.
-static const char * makeImmutableString(std::string_view s)
+StringData * StringData::make(std::string_view s)
 {
-    const size_t size = s.size();
-    if (size == 0)
-        return "";
-    auto t = allocString(size + 1);
-    memcpy(t, s.data(), size);
-    t[size] = '\0';
-    return t;
+    auto res = alloc(s.size());
+    memcpy(&res->m_data, s.data(), s.size());
+    res->m_data[s.size()] = '\0';
+    return res;
 }
 
 RootValue allocRootValue(Value * v)
@@ -584,7 +581,7 @@ std::optional<EvalState::Doc> EvalState::getDoc(Value & v)
             .name = name,
             .arity = 0, // FIXME: figure out how deep by syntax only? It's not semantically useful though...
             .args = {},
-            .doc = makeImmutableString(s.view()), // NOTE: memory leak when compiled without GC
+            .doc = StringData::make(s.view())->view().data(), // NOTE: memory leak when compiled without GC
         };
     }
     if (isFunctor(v)) {
@@ -818,7 +815,7 @@ DebugTraceStacker::DebugTraceStacker(EvalState & evalState, DebugTrace t)
 
 void Value::mkString(std::string_view s)
 {
-    mkStringNoCopy(makeImmutableString(s));
+    mkStringNoCopy(StringData::make(s));
 }
 
 static const char ** encodeContext(const NixStringContext & context)
@@ -827,7 +824,7 @@ static const char ** encodeContext(const NixStringContext & context)
         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++] = StringData::make({i.to_string()})->view().data();
         }
         ctx[n] = nullptr;
         return ctx;
@@ -837,17 +834,17 @@ static const char ** encodeContext(const NixStringContext & context)
 
 void Value::mkString(std::string_view s, const NixStringContext & context)
 {
-    mkStringNoCopy(makeImmutableString(s), encodeContext(context));
+    mkStringNoCopy(StringData::make(s), encodeContext(context));
 }
 
-void Value::mkStringMove(const char * s, const NixStringContext & context)
+void Value::mkStringMove(const StringData * s, const NixStringContext & context)
 {
     mkStringNoCopy(s, encodeContext(context));
 }
 
 void Value::mkPath(const SourcePath & path)
 {
-    mkPath(&*path.accessor, makeImmutableString(path.path.abs()));
+    mkPath(&*path.accessor, StringData::make(path.path.abs()));
 }
 
 inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval)
@@ -1559,6 +1556,7 @@ void EvalState::callFunction(Value & fun, std::span<Value *> args, Value & vRes,
                                 .withFrame(*vCur.lambda().env, lambda)
                                 .debugThrow();
                         }
+
                     unreachable();
                 }
             } else {
@@ -2095,25 +2093,27 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
     } else if (firstType == nFloat) {
         v.mkFloat(nf);
     } else if (firstType == nPath) {
-        if (!context.empty())
+        if (!context.empty()) {
             state.error<EvalError>("a string that refers to a store path cannot be appended to a path")
                 .atPos(pos)
                 .withFrame(env, *this)
                 .debugThrow();
+        }
+
         std::string result_str;
         result_str.reserve(sSize);
         for (const auto & part : strings) {
             result_str += *part;
         }
         v.mkPath(state.rootPath(CanonPath(result_str)));
     } else {
-        char * result_str = allocString(sSize + 1);
-        char * tmp = result_str;
+        auto result_str = StringData::alloc(sSize);
+        auto tmp = result_str->data();
         for (const auto & part : strings) {
             memcpy(tmp, part->data(), part->size());
             tmp += part->size();
         }
-        *tmp = 0;
+        *tmp = '\0';
         v.mkStringMove(result_str, context);
     }
 }
@@ -2366,12 +2366,15 @@ BackedStringView EvalState::coerceToString(
     }
 
     if (v.type() == nPath) {
-        return !canonicalizePath && !copyToStore
-                   ? // FIXME: hack to preserve path literals that end in a
-                     // slash, as in /foo/${x}.
-                   v.pathStr()
-                   : copyToStore ? store->printStorePath(copyPathToStore(context, v.path()))
-                                 : std::string(v.path().path.abs());
+        if (!canonicalizePath && !copyToStore) {
+            // FIXME: hack to preserve path literals that end in a
+            // slash, as in /foo/${x}.
+            return v.pathStr()->view();
+        } else if (copyToStore) {
+            return store->printStorePath(copyPathToStore(context, v.path()));
+        } else {
+            return std::string(v.path().path.abs());
+        }
     }
 
     if (v.type() == nAttrs) {
@@ -2624,7 +2627,7 @@ void EvalState::assertEqValues(Value & v1, Value & v2, const PosIdx pos, std::st
         return;
 
     case nString:
-        if (strcmp(v1.c_str(), v2.c_str()) != 0) {
+        if (v1.string_view() != v2.string_view()) {
             error<AssertionError>(
                 "string '%s' is not equal to string '%s'",
                 ValuePrinter(*this, v1, errorPrintOptions),
@@ -2641,7 +2644,7 @@ void EvalState::assertEqValues(Value & v1, Value & v2, const PosIdx pos, std::st
                 ValuePrinter(*this, v2, errorPrintOptions))
                 .debugThrow();
         }
-        if (strcmp(v1.pathStr(), v2.pathStr()) != 0) {
+        if (v1.pathStr()->view() != v2.pathStr()->view()) {
             error<AssertionError>(
                 "path '%s' is not equal to path '%s'",
                 ValuePrinter(*this, v1, errorPrintOptions),
@@ -2807,12 +2810,12 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v
         return v1.boolean() == v2.boolean();
 
     case nString:
-        return strcmp(v1.c_str(), v2.c_str()) == 0;
+        return v1.string_view() == v2.string_view();
 
     case nPath:
         return
             // FIXME: compare accessors by their fingerprint.
-            v1.pathAccessor() == v2.pathAccessor() && strcmp(v1.pathStr(), v2.pathStr()) == 0;
+            v1.pathAccessor() == v2.pathAccessor() && v1.pathStr()->view() == v2.pathStr()->view();
 
     case nNull:
         return true;

src/libexpr/get-drvs.cc

@@ -168,7 +168,7 @@ PackageInfo::Outputs PackageInfo::queryOutputs(bool withPaths, bool onlyOutputsT
         for (auto elem : outTI->listView()) {
             if (elem->type() != nString)
                 throw errMsg;
-            auto out = outputs.find(elem->c_str());
+            auto out = outputs.find(elem->string_view());
             if (out == outputs.end())
                 throw errMsg;
             result.insert(*out);
@@ -245,7 +245,7 @@ std::string PackageInfo::queryMetaString(const std::string & name)
     Value * v = queryMeta(name);
     if (!v || v->type() != nString)
         return "";
-    return v->c_str();
+    return std::string(v->string_view());
 }
 
 NixInt PackageInfo::queryMetaInt(const std::string & name, NixInt def)
@@ -258,7 +258,7 @@ NixInt PackageInfo::queryMetaInt(const std::string & name, NixInt def)
     if (v->type() == nString) {
         /* Backwards compatibility with before we had support for
            integer meta fields. */
-        if (auto n = string2Int<NixInt::Inner>(v->c_str()))
+        if (auto n = string2Int<NixInt::Inner>(v->string_view()))
             return NixInt{*n};
     }
     return def;
@@ -274,7 +274,7 @@ NixFloat PackageInfo::queryMetaFloat(const std::string & name, NixFloat def)
     if (v->type() == nString) {
         /* Backwards compatibility with before we had support for
            float meta fields. */
-        if (auto n = string2Float<NixFloat>(v->c_str()))
+        if (auto n = string2Float<NixFloat>(v->string_view()))
             return *n;
     }
     return def;

src/libexpr/include/nix/expr/eval-cache.hh

@@ -79,7 +79,11 @@ struct int_t
 
 typedef uint64_t AttrId;
 typedef std::pair<AttrId, Symbol> AttrKey;
-typedef std::pair<std::string, NixStringContext> string_t;
+typedef std::pair<
+    // XXX aspen: StringData?
+    std::string,
+    NixStringContext>
+    string_t;
 
 typedef std::variant<
     std::vector<Symbol>,

src/libexpr/include/nix/expr/get-drvs.hh

@@ -15,7 +15,7 @@ namespace nix {
 struct PackageInfo
 {
 public:
-    typedef std::map<std::string, std::optional<StorePath>> Outputs;
+    typedef std::map<std::string, std::optional<StorePath>, std::less<>> Outputs;
 
 private:
     EvalState * state;