From bd420928730cf268ffe33071292e044118a0c57c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 1 Nov 2025 16:54:22 -0400 Subject: [PATCH] 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 Co-Authored-By: Sergei Zimmerman --- src/libexpr-c/nix_api_value.cc | 2 +- .../include/nix/expr/tests/libexpr.hh | 2 +- src/libexpr-tests/value/value.cc | 18 +++++++++++++++ src/libexpr/eval-cache.cc | 6 ++--- src/libexpr/eval.cc | 23 +++++++++++-------- src/libexpr/get-drvs.cc | 8 +++---- src/libexpr/include/nix/expr/get-drvs.hh | 2 +- src/libexpr/include/nix/expr/value.hh | 7 +++++- src/libexpr/nixexpr.cc | 2 +- src/libexpr/primops.cc | 6 ++--- src/libexpr/value-to-json.cc | 2 +- src/libexpr/value-to-xml.cc | 6 ++--- src/libflake/flake.cc | 6 ++--- src/libutil-c/nix_api_util.cc | 4 ++-- src/libutil-c/nix_api_util_internal.h | 2 +- src/nix/nix-env/nix-env.cc | 6 ++--- 16 files changed, 64 insertions(+), 38 deletions(-) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 3b8c7dd04..e231c36f4 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/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 } diff --git a/src/libexpr-test-support/include/nix/expr/tests/libexpr.hh b/src/libexpr-test-support/include/nix/expr/tests/libexpr.hh index a1320e14a..daae00802 100644 --- a/src/libexpr-test-support/include/nix/expr/tests/libexpr.hh +++ b/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)) diff --git a/src/libexpr-tests/value/value.cc b/src/libexpr-tests/value/value.cc index 63501dd49..229e449db 100644 --- a/src/libexpr-tests/value/value.cc +++ b/src/libexpr-tests/value/value.cc @@ -1,6 +1,7 @@ #include "nix/expr/value.hh" #include "nix/store/tests/libstore.hh" +#include 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 diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 480ca72c7..de74d2143 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -406,7 +406,7 @@ Value & AttrCursor::forceValue() if (root->db && (!cachedValue || std::get_if(&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("'%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 diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 873b88986..e2687148b 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -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.pathStrView(); + } 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( "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( "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; diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 5a7281b2b..c4a2b00af 100644 --- a/src/libexpr/get-drvs.cc +++ b/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(v->c_str())) + if (auto n = string2Int(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(v->c_str())) + if (auto n = string2Float(v->string_view())) return *n; } return def; diff --git a/src/libexpr/include/nix/expr/get-drvs.hh b/src/libexpr/include/nix/expr/get-drvs.hh index 3d42188bf..4beccabe2 100644 --- a/src/libexpr/include/nix/expr/get-drvs.hh +++ b/src/libexpr/include/nix/expr/get-drvs.hh @@ -15,7 +15,7 @@ namespace nix { struct PackageInfo { public: - typedef std::map> Outputs; + typedef std::map, std::less<>> Outputs; private: EvalState * state; diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index 22d85dc99..706a4fe3f 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -1109,7 +1109,7 @@ public: std::string_view string_view() const noexcept { - return std::string_view(getStorage().c_str); + return std::string_view{getStorage().c_str}; } const char * c_str() const noexcept @@ -1177,6 +1177,11 @@ public: return getStorage().path; } + std::string_view pathStrView() const noexcept + { + return std::string_view{getStorage().path}; + } + SourceAccessor * pathAccessor() const noexcept { return getStorage().accessor; diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index b183f1bbf..a1d1b7e4b 100644 --- a/src/libexpr/nixexpr.cc +++ b/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 diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 04196bc1f..96e79fedd 100644 --- a/src/libexpr/primops.cc +++ b/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); } diff --git a/src/libexpr/value-to-json.cc b/src/libexpr/value-to-json.cc index 2cd853f60..03b14b83c 100644 --- a/src/libexpr/value-to-json.cc +++ b/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: diff --git a/src/libexpr/value-to-xml.cc b/src/libexpr/value-to-xml.cc index d5959e894..0a7a334f4 100644 --- a/src/libexpr/value-to-xml.cc +++ b/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); diff --git a/src/libflake/flake.cc b/src/libflake/flake.cc index 42385712c..dc60dbf08 100644 --- a/src/libflake/flake.cc +++ b/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{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"); diff --git a/src/libutil-c/nix_api_util.cc b/src/libutil-c/nix_api_util.cc index 3903823aa..5934e8479 100644 --- a/src/libutil-c/nix_api_util.cc +++ b/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; } diff --git a/src/libutil-c/nix_api_util_internal.h b/src/libutil-c/nix_api_util_internal.h index 92bb9c1d2..e4c5e93bb 100644 --- a/src/libutil-c/nix_api_util_internal.h +++ b/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 (...) \ diff --git a/src/nix/nix-env/nix-env.cc b/src/nix/nix-env/nix-env.cc index 01c8ccf4b..2a0984d18 100644 --- a/src/nix/nix-env/nix-env.cc +++ b/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); } }