Use less c_str() in the evaluator, and other cleanups

It is better to avoid null termination for performance and memory
safety, wherever possible.

These are good cleanups extracted from the Pascal String work that we
can land by themselves first, shrinking the diff in that PR.

Co-Authored-By: Aspen Smith <root@gws.fyi>
Co-Authored-By: Sergei Zimmerman <sergei@zimmerman.foo>

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
 }

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

@@ -106,7 +106,7 @@ MATCHER_P(IsStringEq, s, fmt("The string is equal to \"%1%\"", s))
     if (arg.type() != nString) {
         return false;
     }
-    return std::string_view(arg.c_str()) == s;
+    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_view();
+    auto sd2 = vStr2.string_view();
+
+    // The strings should be the same
+    ASSERT_EQ(sd1, sd2);
+
+    // The strings should also be backed by the same (static) allocation
+    ASSERT_EQ(sd1.data(), sd2.data());
+}
+
 } // 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

@@ -2366,12 +2366,15 @@ BackedStringView EvalState::coerceToString(
     }
 
     if (v.type() == nPath) {
-        return !canonicalizePath && !copyToStore
+        if (!canonicalizePath && !copyToStore) {
-                   ? // FIXME: hack to preserve path literals that end in a
+            // FIXME: hack to preserve path literals that end in a
             // slash, as in /foo/${x}.
-                   v.pathStr()
+            return v.pathStrView();
-                   : copyToStore ? store->printStorePath(copyPathToStore(context, v.path()))
+        } else if (copyToStore) {
-                                 : std::string(v.path().path.abs());
+            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.pathStrView() != v2.pathStrView()) {
             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.pathStrView() == v2.pathStrView();
 
     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/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;

src/libexpr/include/nix/expr/value.hh

@@ -1109,7 +1109,7 @@ public:
 
     std::string_view string_view() const noexcept
     {
-        return std::string_view(getStorage<StringWithContext>().c_str);
+        return std::string_view{getStorage<StringWithContext>().c_str};
     }
 
     const char * c_str() const noexcept
@@ -1177,6 +1177,11 @@ public:
         return getStorage<Path>().path;
     }
 
+    std::string_view pathStrView() const noexcept
+    {
+        return std::string_view{getStorage<Path>().path};
+    }
+
     SourceAccessor * pathAccessor() const noexcept
     {
         return getStorage<Path>().accessor;

src/libexpr/nixexpr.cc

@@ -45,7 +45,7 @@ void ExprString::show(const SymbolTable & symbols, std::ostream & str) const
 
 void ExprPath::show(const SymbolTable & symbols, std::ostream & str) const
 {
-    str << v.pathStr();
+    str << v.pathStrView();
 }
 
 void ExprVar::show(const SymbolTable & symbols, std::ostream & str) const

src/libexpr/primops.cc

@@ -691,12 +691,12 @@ struct CompareValues
             case nFloat:
                 return v1->fpoint() < v2->fpoint();
             case nString:
-                return strcmp(v1->c_str(), v2->c_str()) < 0;
+                return v1->string_view() < v2->string_view();
             case nPath:
                 // Note: we don't take the accessor into account
                 // since it's not obvious how to compare them in a
                 // reproducible way.
-                return strcmp(v1->pathStr(), v2->pathStr()) < 0;
+                return v1->pathStrView() < v2->pathStrView();
             case nList:
                 // Lexicographic comparison
                 for (size_t i = 0;; i++) {
@@ -2930,7 +2930,7 @@ static void prim_attrNames(EvalState & state, const PosIdx pos, Value ** args, V
     for (const auto & [n, i] : enumerate(*args[0]->attrs()))
         list[n] = Value::toPtr(state.symbols[i.name]);
 
-    std::sort(list.begin(), list.end(), [](Value * v1, Value * v2) { return strcmp(v1->c_str(), v2->c_str()) < 0; });
+    std::sort(list.begin(), list.end(), [](Value * v1, Value * v2) { return v1->string_view() < v2->string_view(); });
 
     v.mkList(list);
 }

src/libexpr/value-to-json.cc

@@ -33,7 +33,7 @@ json printValueAsJSON(
 
     case nString:
         copyContext(v, context);
-        out = v.c_str();
+        out = v.string_view();
         break;
 
     case nPath:

src/libexpr/value-to-xml.cc

@@ -82,7 +82,7 @@ static void printValueAsXML(
     case nString:
         /* !!! show the context? */
         copyContext(v, context);
-        doc.writeEmptyElement("string", singletonAttrs("value", v.c_str()));
+        doc.writeEmptyElement("string", singletonAttrs("value", v.string_view()));
         break;
 
     case nPath:
@@ -102,14 +102,14 @@ static void printValueAsXML(
                 if (strict)
                     state.forceValue(*a->value, a->pos);
                 if (a->value->type() == nString)
-                    xmlAttrs["drvPath"] = drvPath = a->value->c_str();
+                    xmlAttrs["drvPath"] = drvPath = a->value->string_view();
             }
 
             if (auto a = v.attrs()->get(state.s.outPath)) {
                 if (strict)
                     state.forceValue(*a->value, a->pos);
                 if (a->value->type() == nString)
-                    xmlAttrs["outPath"] = a->value->c_str();
+                    xmlAttrs["outPath"] = a->value->string_view();
             }
 
             XMLOpenElement _(doc, "derivation", xmlAttrs);

src/libflake/flake.cc

@@ -97,7 +97,7 @@ static void parseFlakeInputAttr(EvalState & state, const Attr & attr, fetchers::
 #pragma GCC diagnostic ignored "-Wswitch-enum"
     switch (attr.value->type()) {
     case nString:
-        attrs.emplace(state.symbols[attr.name], attr.value->c_str());
+        attrs.emplace(state.symbols[attr.name], std::string(attr.value->string_view()));
         break;
     case nBool:
         attrs.emplace(state.symbols[attr.name], Explicit<bool>{attr.value->boolean()});
@@ -177,7 +177,7 @@ static FlakeInput parseFlakeInput(
                     parseFlakeInputs(state, attr.value, attr.pos, lockRootAttrPath, flakeDir, false).first;
             } else if (attr.name == sFollows) {
                 expectType(state, nString, *attr.value, attr.pos);
-                auto follows(parseInputAttrPath(attr.value->c_str()));
+                auto follows(parseInputAttrPath(attr.value->string_view()));
                 follows.insert(follows.begin(), lockRootAttrPath.begin(), lockRootAttrPath.end());
                 input.follows = follows;
             } else
@@ -264,7 +264,7 @@ static Flake readFlake(
 
     if (auto description = vInfo.attrs()->get(state.s.description)) {
         expectType(state, nString, *description->value, description->pos);
-        flake.description = description->value->c_str();
+        flake.description = description->value->string_view();
     }
 
     auto sInputs = state.symbols.create("inputs");

src/libutil-c/nix_api_util.cc

@@ -153,9 +153,9 @@ nix_err nix_err_code(const nix_c_context * read_context)
 }
 
 // internal
-nix_err call_nix_get_string_callback(const std::string str, nix_get_string_callback callback, void * user_data)
+nix_err call_nix_get_string_callback(const std::string_view str, nix_get_string_callback callback, void * user_data)
 {
-    callback(str.c_str(), str.size(), user_data);
+    callback(str.data(), str.size(), user_data);
     return NIX_OK;
 }
 

src/libutil-c/nix_api_util_internal.h

@@ -32,7 +32,7 @@ nix_err nix_context_error(nix_c_context * context);
  * @return NIX_OK if there were no errors.
  * @see nix_get_string_callback
  */
-nix_err call_nix_get_string_callback(const std::string str, nix_get_string_callback callback, void * user_data);
+nix_err call_nix_get_string_callback(const std::string_view str, nix_get_string_callback callback, void * user_data);
 
 #define NIXC_CATCH_ERRS                    \
     catch (...)                            \

src/nix/nix-env/nix-env.cc

@@ -1228,7 +1228,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                         else {
                             if (v->type() == nString) {
                                 attrs2["type"] = "string";
-                                attrs2["value"] = v->c_str();
+                                attrs2["value"] = v->string_view();
                                 xml.writeEmptyElement("meta", attrs2);
                             } else if (v->type() == nInt) {
                                 attrs2["type"] = "int";
@@ -1249,7 +1249,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                                     if (elem->type() != nString)
                                         continue;
                                     XMLAttrs attrs3;
-                                    attrs3["value"] = elem->c_str();
+                                    attrs3["value"] = elem->string_view();
                                     xml.writeEmptyElement("string", attrs3);
                                 }
                             } else if (v->type() == nAttrs) {
@@ -1260,7 +1260,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                                         continue;
                                     XMLAttrs attrs3;
                                     attrs3["type"] = globals.state->symbols[i.name];
-                                    attrs3["value"] = i.value->c_str();
+                                    attrs3["value"] = i.value->string_view();
                                     xml.writeEmptyElement("string", attrs3);
                                 }
                             }