optionally return string_view from coerceToString

we'll retain the old coerceToString interface that returns a string, but callers
that don't need the returned value to outlive the Value it came from can save
copies by using the new interface instead. for values that weren't stringy we'll
pass a new buffer argument that'll be used for storage and shouldn't be
inspected.
This commit is contained in:
pennae 2022-01-21 16:20:54 +01:00
parent 41d70a2fc8
commit d439dceb3b
10 changed files with 121 additions and 51 deletions

View file

@ -1,5 +1,6 @@
#include "eval.hh" #include "eval.hh"
#include "hash.hh" #include "hash.hh"
#include "types.hh"
#include "util.hh" #include "util.hh"
#include "store-api.hh" #include "store-api.hh"
#include "derivations.hh" #include "derivations.hh"
@ -1694,7 +1695,7 @@ void EvalState::concatLists(Value & v, size_t nrLists, Value * * lists, const Po
void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
{ {
PathSet context; PathSet context;
std::vector<std::string> s; std::vector<BackedStringView> s;
size_t sSize = 0; size_t sSize = 0;
NixInt n = 0; NixInt n = 0;
NixFloat nf = 0; NixFloat nf = 0;
@ -1705,7 +1706,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
const auto str = [&] { const auto str = [&] {
std::string result; std::string result;
result.reserve(sSize); result.reserve(sSize);
for (const auto & part : s) result += part; for (const auto & part : s) result += *part;
return result; return result;
}; };
/* c_str() is not str().c_str() because we want to create a string /* c_str() is not str().c_str() because we want to create a string
@ -1715,15 +1716,18 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
char * result = allocString(sSize + 1); char * result = allocString(sSize + 1);
char * tmp = result; char * tmp = result;
for (const auto & part : s) { for (const auto & part : s) {
memcpy(tmp, part.c_str(), part.size()); memcpy(tmp, part->data(), part->size());
tmp += part.size(); tmp += part->size();
} }
*tmp = 0; *tmp = 0;
return result; return result;
}; };
Value values[es->size()];
Value * vTmpP = values;
for (auto & [i_pos, i] : *es) { for (auto & [i_pos, i] : *es) {
Value vTmp; Value & vTmp = *vTmpP++;
i->eval(state, env, vTmp); i->eval(state, env, vTmp);
/* If the first element is a path, then the result will also /* If the first element is a path, then the result will also
@ -1756,9 +1760,9 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
/* skip canonization of first path, which would only be not /* skip canonization of first path, which would only be not
canonized in the first place if it's coming from a ./${foo} type canonized in the first place if it's coming from a ./${foo} type
path */ path */
s.emplace_back( auto part = state.coerceToString(i_pos, vTmp, context, false, firstType == nString, !first);
state.coerceToString(i_pos, vTmp, context, false, firstType == nString, !first)); sSize += part->size();
sSize += s.back().size(); s.emplace_back(std::move(part));
} }
first = false; first = false;
@ -1942,34 +1946,35 @@ std::optional<string> EvalState::tryAttrsToString(const Pos & pos, Value & v,
if (i != v.attrs->end()) { if (i != v.attrs->end()) {
Value v1; Value v1;
callFunction(*i->value, v, v1, pos); callFunction(*i->value, v, v1, pos);
return coerceToString(pos, v1, context, coerceMore, copyToStore); return coerceToString(pos, v1, context, coerceMore, copyToStore).toOwned();
} }
return {}; return {};
} }
string EvalState::coerceToString(const Pos & pos, Value & v, PathSet & context, BackedStringView EvalState::coerceToString(const Pos & pos, Value & v, PathSet & context,
bool coerceMore, bool copyToStore, bool canonicalizePath) bool coerceMore, bool copyToStore, bool canonicalizePath)
{ {
forceValue(v, pos); forceValue(v, pos);
string s;
if (v.type() == nString) { if (v.type() == nString) {
copyContext(v, context); copyContext(v, context);
return v.string.s; return std::string_view(v.string.s);
} }
if (v.type() == nPath) { if (v.type() == nPath) {
Path path(canonicalizePath ? canonPath(v.path) : v.path); BackedStringView path(PathView(v.path));
return copyToStore ? copyPathToStore(context, path) : path; if (canonicalizePath)
path = canonPath(*path);
if (copyToStore)
path = copyPathToStore(context, std::move(path).toOwned());
return path;
} }
if (v.type() == nAttrs) { if (v.type() == nAttrs) {
auto maybeString = tryAttrsToString(pos, v, context, coerceMore, copyToStore); auto maybeString = tryAttrsToString(pos, v, context, coerceMore, copyToStore);
if (maybeString) { if (maybeString)
return *maybeString; return std::move(*maybeString);
}
auto i = v.attrs->find(sOutPath); auto i = v.attrs->find(sOutPath);
if (i == v.attrs->end()) throwTypeError(pos, "cannot coerce a set to a string"); if (i == v.attrs->end()) throwTypeError(pos, "cannot coerce a set to a string");
return coerceToString(pos, *i->value, context, coerceMore, copyToStore); return coerceToString(pos, *i->value, context, coerceMore, copyToStore);
@ -1991,14 +1996,13 @@ string EvalState::coerceToString(const Pos & pos, Value & v, PathSet & context,
if (v.isList()) { if (v.isList()) {
string result; string result;
for (auto [n, v2] : enumerate(v.listItems())) { for (auto [n, v2] : enumerate(v.listItems())) {
result += coerceToString(pos, *v2, result += *coerceToString(pos, *v2, context, coerceMore, copyToStore);
context, coerceMore, copyToStore);
if (n < v.listSize() - 1 if (n < v.listSize() - 1
/* !!! not quite correct */ /* !!! not quite correct */
&& (!v2->isList() || v2->listSize() != 0)) && (!v2->isList() || v2->listSize() != 0))
result += " "; result += " ";
} }
return result; return std::move(result);
} }
} }
@ -2032,7 +2036,7 @@ string EvalState::copyPathToStore(PathSet & context, const Path & path)
Path EvalState::coerceToPath(const Pos & pos, Value & v, PathSet & context) Path EvalState::coerceToPath(const Pos & pos, Value & v, PathSet & context)
{ {
string path = coerceToString(pos, v, context, false, false); string path = coerceToString(pos, v, context, false, false).toOwned();
if (path == "" || path[0] != '/') if (path == "" || path[0] != '/')
throwEvalError(pos, "string '%1%' doesn't represent an absolute path", path); throwEvalError(pos, "string '%1%' doesn't represent an absolute path", path);
return path; return path;

View file

@ -1,6 +1,7 @@
#pragma once #pragma once
#include "attr-set.hh" #include "attr-set.hh"
#include "types.hh"
#include "value.hh" #include "value.hh"
#include "nixexpr.hh" #include "nixexpr.hh"
#include "symbol-table.hh" #include "symbol-table.hh"
@ -251,7 +252,7 @@ public:
string. If `coerceMore' is set, also converts nulls, integers, string. If `coerceMore' is set, also converts nulls, integers,
booleans and lists to a string. If `copyToStore' is set, booleans and lists to a string. If `copyToStore' is set,
referenced paths are copied to the Nix store as a side effect. */ referenced paths are copied to the Nix store as a side effect. */
string coerceToString(const Pos & pos, Value & v, PathSet & context, BackedStringView coerceToString(const Pos & pos, Value & v, PathSet & context,
bool coerceMore = false, bool copyToStore = true, bool coerceMore = false, bool copyToStore = true,
bool canonicalizePath = true); bool canonicalizePath = true);

View file

@ -253,7 +253,9 @@ static Flake getFlake(
flake.config.settings.insert({setting.name, string(state.forceStringNoCtx(*setting.value, *setting.pos))}); flake.config.settings.insert({setting.name, string(state.forceStringNoCtx(*setting.value, *setting.pos))});
else if (setting.value->type() == nPath) { else if (setting.value->type() == nPath) {
PathSet emptyContext = {}; PathSet emptyContext = {};
flake.config.settings.insert({setting.name, state.coerceToString(*setting.pos, *setting.value, emptyContext, false, true, true)}); flake.config.settings.emplace(
setting.name,
state.coerceToString(*setting.pos, *setting.value, emptyContext, false, true, true) .toOwned());
} }
else if (setting.value->type() == nInt) else if (setting.value->type() == nInt)
flake.config.settings.insert({setting.name, state.forceInt(*setting.value, *setting.pos)}); flake.config.settings.insert({setting.name, state.forceInt(*setting.value, *setting.pos)});

View file

@ -350,10 +350,11 @@ void prim_exec(EvalState & state, const Pos & pos, Value * * args, Value & v)
}); });
} }
PathSet context; PathSet context;
auto program = state.coerceToString(pos, *elems[0], context, false, false); auto program = state.coerceToString(pos, *elems[0], context, false, false).toOwned();
Strings commandArgs; Strings commandArgs;
for (unsigned int i = 1; i < args[0]->listSize(); ++i) for (unsigned int i = 1; i < args[0]->listSize(); ++i) {
commandArgs.emplace_back(state.coerceToString(pos, *elems[i], context, false, false)); commandArgs.push_back(state.coerceToString(pos, *elems[i], context, false, false).toOwned());
}
try { try {
auto _ = state.realiseContext(context); // FIXME: Handle CA derivations auto _ = state.realiseContext(context); // FIXME: Handle CA derivations
} catch (InvalidPathError & e) { } catch (InvalidPathError & e) {
@ -706,7 +707,7 @@ static RegisterPrimOp primop_abort({
.fun = [](EvalState & state, const Pos & pos, Value * * args, Value & v) .fun = [](EvalState & state, const Pos & pos, Value * * args, Value & v)
{ {
PathSet context; PathSet context;
string s = state.coerceToString(pos, *args[0], context); string s = state.coerceToString(pos, *args[0], context).toOwned();
throw Abort("evaluation aborted with the following error message: '%1%'", s); throw Abort("evaluation aborted with the following error message: '%1%'", s);
} }
}); });
@ -724,7 +725,7 @@ static RegisterPrimOp primop_throw({
.fun = [](EvalState & state, const Pos & pos, Value * * args, Value & v) .fun = [](EvalState & state, const Pos & pos, Value * * args, Value & v)
{ {
PathSet context; PathSet context;
string s = state.coerceToString(pos, *args[0], context); string s = state.coerceToString(pos, *args[0], context).toOwned();
throw ThrownError(s); throw ThrownError(s);
} }
}); });
@ -736,7 +737,7 @@ static void prim_addErrorContext(EvalState & state, const Pos & pos, Value * * a
v = *args[1]; v = *args[1];
} catch (Error & e) { } catch (Error & e) {
PathSet context; PathSet context;
e.addTrace(std::nullopt, state.coerceToString(pos, *args[0], context)); e.addTrace(std::nullopt, state.coerceToString(pos, *args[0], context).toOwned());
throw; throw;
} }
} }
@ -1030,7 +1031,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * *
else if (i->name == state.sArgs) { else if (i->name == state.sArgs) {
state.forceList(*i->value, pos); state.forceList(*i->value, pos);
for (auto elem : i->value->listItems()) { for (auto elem : i->value->listItems()) {
string s = state.coerceToString(posDrvName, *elem, context, true); string s = state.coerceToString(posDrvName, *elem, context, true).toOwned();
drv.args.push_back(s); drv.args.push_back(s);
} }
} }
@ -1066,7 +1067,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * *
} }
} else { } else {
auto s = state.coerceToString(*i->pos, *i->value, context, true); auto s = state.coerceToString(*i->pos, *i->value, context, true).toOwned();
drv.env.emplace(key, s); drv.env.emplace(key, s);
if (i->name == state.sBuilder) drv.builder = std::move(s); if (i->name == state.sBuilder) drv.builder = std::move(s);
else if (i->name == state.sSystem) drv.platform = std::move(s); else if (i->name == state.sSystem) drv.platform = std::move(s);
@ -1399,7 +1400,7 @@ static RegisterPrimOp primop_pathExists({
static void prim_baseNameOf(EvalState & state, const Pos & pos, Value * * args, Value & v) static void prim_baseNameOf(EvalState & state, const Pos & pos, Value * * args, Value & v)
{ {
PathSet context; PathSet context;
v.mkString(baseNameOf(state.coerceToString(pos, *args[0], context, false, false)), context); v.mkString(baseNameOf(*state.coerceToString(pos, *args[0], context, false, false)), context);
} }
static RegisterPrimOp primop_baseNameOf({ static RegisterPrimOp primop_baseNameOf({
@ -1419,7 +1420,8 @@ static RegisterPrimOp primop_baseNameOf({
static void prim_dirOf(EvalState & state, const Pos & pos, Value * * args, Value & v) static void prim_dirOf(EvalState & state, const Pos & pos, Value * * args, Value & v)
{ {
PathSet context; PathSet context;
Path dir = dirOf(state.coerceToString(pos, *args[0], context, false, false)); auto path = state.coerceToString(pos, *args[0], context, false, false);
auto dir = dirOf(*path);
if (args[0]->type() == nPath) v.mkPath(dir); else v.mkString(dir, context); if (args[0]->type() == nPath) v.mkPath(dir); else v.mkString(dir, context);
} }
@ -1486,7 +1488,7 @@ static void prim_findFile(EvalState & state, const Pos & pos, Value * * args, Va
); );
PathSet context; PathSet context;
string path = state.coerceToString(pos, *i->value, context, false, false); string path = state.coerceToString(pos, *i->value, context, false, false).toOwned();
try { try {
auto rewrites = state.realiseContext(context); auto rewrites = state.realiseContext(context);
@ -3288,8 +3290,8 @@ static RegisterPrimOp primop_lessThan({
static void prim_toString(EvalState & state, const Pos & pos, Value * * args, Value & v) static void prim_toString(EvalState & state, const Pos & pos, Value * * args, Value & v)
{ {
PathSet context; PathSet context;
string s = state.coerceToString(pos, *args[0], context, true, false); auto s = state.coerceToString(pos, *args[0], context, true, false);
v.mkString(s, context); v.mkString(*s, context);
} }
static RegisterPrimOp primop_toString({ static RegisterPrimOp primop_toString({
@ -3325,7 +3327,7 @@ static void prim_substring(EvalState & state, const Pos & pos, Value * * args, V
int start = state.forceInt(*args[0], pos); int start = state.forceInt(*args[0], pos);
int len = state.forceInt(*args[1], pos); int len = state.forceInt(*args[1], pos);
PathSet context; PathSet context;
string s = state.coerceToString(pos, *args[2], context); auto s = state.coerceToString(pos, *args[2], context);
if (start < 0) if (start < 0)
throw EvalError({ throw EvalError({
@ -3333,7 +3335,7 @@ static void prim_substring(EvalState & state, const Pos & pos, Value * * args, V
.errPos = pos .errPos = pos
}); });
v.mkString((unsigned int) start >= s.size() ? "" : string(s, start, len), context); v.mkString((unsigned int) start >= s->size() ? "" : s->substr(start, len), context);
} }
static RegisterPrimOp primop_substring({ static RegisterPrimOp primop_substring({
@ -3359,8 +3361,8 @@ static RegisterPrimOp primop_substring({
static void prim_stringLength(EvalState & state, const Pos & pos, Value * * args, Value & v) static void prim_stringLength(EvalState & state, const Pos & pos, Value * * args, Value & v)
{ {
PathSet context; PathSet context;
string s = state.coerceToString(pos, *args[0], context); auto s = state.coerceToString(pos, *args[0], context);
v.mkInt(s.size()); v.mkInt(s->size());
} }
static RegisterPrimOp primop_stringLength({ static RegisterPrimOp primop_stringLength({
@ -3620,7 +3622,7 @@ static void prim_concatStringsSep(EvalState & state, const Pos & pos, Value * *
for (auto elem : args[1]->listItems()) { for (auto elem : args[1]->listItems()) {
if (first) first = false; else res += sep; if (first) first = false; else res += sep;
res += state.coerceToString(pos, *elem, context); res += *state.coerceToString(pos, *elem, context);
} }
v.mkString(res, context); v.mkString(res, context);

View file

@ -7,7 +7,8 @@ namespace nix {
static void prim_unsafeDiscardStringContext(EvalState & state, const Pos & pos, Value * * args, Value & v) static void prim_unsafeDiscardStringContext(EvalState & state, const Pos & pos, Value * * args, Value & v)
{ {
PathSet context; PathSet context;
v.mkString(state.coerceToString(pos, *args[0], context)); auto s = state.coerceToString(pos, *args[0], context);
v.mkString(*s);
} }
static RegisterPrimOp primop_unsafeDiscardStringContext("__unsafeDiscardStringContext", 1, prim_unsafeDiscardStringContext); static RegisterPrimOp primop_unsafeDiscardStringContext("__unsafeDiscardStringContext", 1, prim_unsafeDiscardStringContext);
@ -32,13 +33,13 @@ static RegisterPrimOp primop_hasContext("__hasContext", 1, prim_hasContext);
static void prim_unsafeDiscardOutputDependency(EvalState & state, const Pos & pos, Value * * args, Value & v) static void prim_unsafeDiscardOutputDependency(EvalState & state, const Pos & pos, Value * * args, Value & v)
{ {
PathSet context; PathSet context;
string s = state.coerceToString(pos, *args[0], context); auto s = state.coerceToString(pos, *args[0], context);
PathSet context2; PathSet context2;
for (auto & p : context) for (auto & p : context)
context2.insert(p.at(0) == '=' ? string(p, 1) : p); context2.insert(p.at(0) == '=' ? string(p, 1) : p);
v.mkString(s, context2); v.mkString(*s, context2);
} }
static RegisterPrimOp primop_unsafeDiscardOutputDependency("__unsafeDiscardOutputDependency", 1, prim_unsafeDiscardOutputDependency); static RegisterPrimOp primop_unsafeDiscardOutputDependency("__unsafeDiscardOutputDependency", 1, prim_unsafeDiscardOutputDependency);

View file

@ -24,7 +24,7 @@ static void prim_fetchMercurial(EvalState & state, const Pos & pos, Value * * ar
for (auto & attr : *args[0]->attrs) { for (auto & attr : *args[0]->attrs) {
std::string_view n(attr.name); std::string_view n(attr.name);
if (n == "url") if (n == "url")
url = state.coerceToString(*attr.pos, *attr.value, context, false, false); url = state.coerceToString(*attr.pos, *attr.value, context, false, false).toOwned();
else if (n == "rev") { else if (n == "rev") {
// Ugly: unlike fetchGit, here the "rev" attribute can // Ugly: unlike fetchGit, here the "rev" attribute can
// be both a revision or a branch/tag name. // be both a revision or a branch/tag name.
@ -50,7 +50,7 @@ static void prim_fetchMercurial(EvalState & state, const Pos & pos, Value * * ar
}); });
} else } else
url = state.coerceToString(pos, *args[0], context, false, false); url = state.coerceToString(pos, *args[0], context, false, false).toOwned();
// FIXME: git externals probably can be used to bypass the URI // FIXME: git externals probably can be used to bypass the URI
// whitelist. Ah well. // whitelist. Ah well.

View file

@ -125,7 +125,7 @@ static void fetchTree(
if (attr.name == state.sType) continue; if (attr.name == state.sType) continue;
state.forceValue(*attr.value, *attr.pos); state.forceValue(*attr.value, *attr.pos);
if (attr.value->type() == nPath || attr.value->type() == nString) { if (attr.value->type() == nPath || attr.value->type() == nString) {
auto s = state.coerceToString(*attr.pos, *attr.value, context, false, false); auto s = state.coerceToString(*attr.pos, *attr.value, context, false, false).toOwned();
attrs.emplace(attr.name, attrs.emplace(attr.name,
attr.name == "url" attr.name == "url"
? type == "git" ? type == "git"
@ -151,7 +151,7 @@ static void fetchTree(
input = fetchers::Input::fromAttrs(std::move(attrs)); input = fetchers::Input::fromAttrs(std::move(attrs));
} else { } else {
auto url = state.coerceToString(pos, *args[0], context, false, false); auto url = state.coerceToString(pos, *args[0], context, false, false).toOwned();
if (type == "git") { if (type == "git") {
fetchers::Attrs attrs; fetchers::Attrs attrs;

View file

@ -6,6 +6,7 @@
#include <set> #include <set>
#include <string> #include <string>
#include <map> #include <map>
#include <variant>
#include <vector> #include <vector>
namespace nix { namespace nix {
@ -47,4 +48,63 @@ struct Explicit {
} }
}; };
/* This wants to be a little bit like rust's Cow type.
Some parts of the evaluator benefit greatly from being able to reuse
existing allocations for strings, but have to be able to also use
newly allocated storage for values.
We do not define implicit conversions, even with ref qualifiers,
since those can easily become ambiguous to the reader and can degrade
into copying behaviour we want to avoid. */
class BackedStringView {
private:
std::variant<std::string, std::string_view> data;
/* Needed to introduce a temporary since operator-> must return
a pointer. Without this we'd need to store the view object
even when we already own a string. */
class Ptr {
private:
std::string_view view;
public:
Ptr(std::string_view view): view(view) {}
const std::string_view * operator->() const { return &view; }
};
public:
BackedStringView(std::string && s): data(std::move(s)) {}
BackedStringView(std::string_view sv): data(sv) {}
template<size_t N>
BackedStringView(const char (& lit)[N]): data(std::string_view(lit)) {}
BackedStringView(const BackedStringView &) = delete;
BackedStringView & operator=(const BackedStringView &) = delete;
/* We only want move operations defined since the sole purpose of
this type is to avoid copies. */
BackedStringView(BackedStringView && other) = default;
BackedStringView & operator=(BackedStringView && other) = default;
bool isOwned() const
{
return std::holds_alternative<std::string>(data);
}
std::string toOwned() &&
{
return isOwned()
? std::move(std::get<std::string>(data))
: std::string(std::get<std::string_view>(data));
}
std::string_view operator*() const
{
return isOwned()
? std::get<std::string>(data)
: std::get<std::string_view>(data);
}
Ptr operator->() const { return Ptr(**this); }
};
} }

View file

@ -107,7 +107,7 @@ struct CmdEval : MixJSON, InstallableCommand
else if (raw) { else if (raw) {
stopProgressBar(); stopProgressBar();
std::cout << state->coerceToString(noPos, *v, context); std::cout << *state->coerceToString(noPos, *v, context);
} }
else if (json) { else if (json) {

View file

@ -463,7 +463,7 @@ bool NixRepl::processLine(string line)
if (v.type() == nPath || v.type() == nString) { if (v.type() == nPath || v.type() == nString) {
PathSet context; PathSet context;
auto filename = state->coerceToString(noPos, v, context); auto filename = state->coerceToString(noPos, v, context);
pos.file = state->symbols.create(filename); pos.file = state->symbols.create(*filename);
} else if (v.isLambda()) { } else if (v.isLambda()) {
pos = v.lambda.fun->pos; pos = v.lambda.fun->pos;
} else { } else {