Merge pull request #14444 from NixOS/less-c_str

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

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}.
+            // 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);
                                 }
                             }