From 1eff2adc93dacb6a647bc9701ffa7437e1eccde6 Mon Sep 17 00:00:00 2001 From: Mel Zuser Date: Wed, 15 May 2024 20:03:19 -0700 Subject: [PATCH] libexpr: abstract string members in Value to allow multiple internal reps similar to how lists works Change-Id: I94139aab28b04b73b055a3dc05d496758ee72538 --- src/libcmd/repl.cc | 2 +- src/libexpr/eval-cache.cc | 8 +++--- src/libexpr/eval.cc | 20 +++++++------- src/libexpr/flake/flake.cc | 8 +++--- src/libexpr/get-drvs.cc | 12 ++++----- src/libexpr/primops.cc | 10 +++---- src/libexpr/primops/fetchClosure.cc | 2 +- src/libexpr/print-ambiguous.cc | 2 +- src/libexpr/print.cc | 2 +- src/libexpr/value-to-json.cc | 2 +- src/libexpr/value-to-xml.cc | 6 ++--- src/libexpr/value.hh | 30 ++++++++++++++++++--- src/nix-env/nix-env.cc | 6 ++--- src/nix/eval.cc | 2 +- tests/unit/libexpr-support/tests/libexpr.hh | 4 +-- tests/unit/libexpr/primops.cc | 4 +-- 16 files changed, 72 insertions(+), 48 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 525c25560..35b802d89 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -674,7 +674,7 @@ ProcessLineResult NixRepl::processLine(std::string line) Value v; evalString(arg, v); if (v.type() == nString) { - std::cout << v.string.s; + std::cout << v.c_str(); } else { printValue(std::cout, v); } diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 5969ee449..ffe9e1bab 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -440,8 +440,8 @@ Value & AttrCursor::forceValue() if (root->db && (!cachedValue || std::get_if(&cachedValue->second))) { if (v.type() == nString) - cachedValue = {root->db->setString(getKey(), v.string.s, v.string.context), - string_t{v.string.s, {}}}; + cachedValue = {root->db->setString(getKey(), v.c_str(), v.stringContext()), + string_t{v.c_str(), {}}}; else if (v.type() == nPath) { auto path = v.path().path; cachedValue = {root->db->setString(getKey(), path.abs()), string_t{path.abs(), {}}}; @@ -580,7 +580,7 @@ std::string AttrCursor::getString() root->state.error("'%s' is not a string but %s", getAttrPathStr(), v.type()).debugThrow(); } - return v.type() == nString ? v.string.s : v.path().to_string(); + return v.type() == nString ? v.c_str() : v.path().to_string(); } string_t AttrCursor::getStringWithContext() @@ -622,7 +622,7 @@ string_t AttrCursor::getStringWithContext() if (v.type() == nString) { NixStringContext context; copyContext(v, context); - return {v.string.s, std::move(context)}; + return {v.c_str(), 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 65f0a7938..d1f3e59d9 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -253,7 +253,7 @@ static Symbol getName(const AttrName & name, EvalState & state, Env & env) Value nameValue; name.expr->eval(state, env, nameValue); state.forceStringNoCtx(nameValue, name.expr->getPos(), "while evaluating an attribute name"); - return state.symbols.create(nameValue.string.s); + return state.symbols.create(nameValue.c_str()); } } @@ -1330,7 +1330,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) if (nameVal.type() == nNull) continue; state.forceStringNoCtx(nameVal, i.pos, "while evaluating the name of a dynamic attribute"); - auto nameSym = state.symbols.create(nameVal.string.s); + auto nameSym = state.symbols.create(nameVal.c_str()); Bindings::iterator j = v.attrs->find(nameSym); if (j != v.attrs->end()) state.error("dynamic attribute '%1%' already defined at %2%", state.symbols[nameSym], state.positions[j->pos]).atPos(i.pos).withFrame(env, *this).debugThrow(); @@ -2256,7 +2256,7 @@ std::string_view EvalState::forceString(Value & v, const PosIdx pos, std::string showType(v), ValuePrinter(*this, v, errorPrintOptions) ).atPos(pos).debugThrow(); - return v.string.s; + return v.str(); } catch (Error & e) { e.addTrace(positions[pos], errorCtx); throw; @@ -2266,8 +2266,8 @@ std::string_view EvalState::forceString(Value & v, const PosIdx pos, std::string void copyContext(const Value & v, NixStringContext & context) { - if (v.string.context) - for (const char * * p = v.string.context; *p; ++p) + if (v.stringContext()) + for (const char * * p = v.stringContext(); *p; ++p) context.insert(NixStringContextElem::parse(*p)); } @@ -2283,8 +2283,8 @@ std::string_view EvalState::forceString(Value & v, NixStringContext & context, c std::string_view EvalState::forceStringNoCtx(Value & v, const PosIdx pos, std::string_view errorCtx) { auto s = forceString(v, pos, errorCtx); - if (v.string.context) { - error("the string '%1%' is not allowed to refer to a store path (such as '%2%')", v.string.s, v.string.context[0]).withTrace(pos, errorCtx).debugThrow(); + if (v.stringContext()) { + error("the string '%1%' is not allowed to refer to a store path (such as '%2%')", v.c_str(), v.stringContext()[0]).withTrace(pos, errorCtx).debugThrow(); } return s; } @@ -2297,7 +2297,7 @@ bool EvalState::isDerivation(Value & v) if (i == v.attrs->end()) return false; forceValue(*i->value, i->pos); if (i->value->type() != nString) return false; - return strcmp(i->value->string.s, "derivation") == 0; + return strcmp(i->value->c_str(), "derivation") == 0; } @@ -2329,7 +2329,7 @@ BackedStringView EvalState::coerceToString( if (v.type() == nString) { copyContext(v, context); - return std::string_view(v.string.s); + return v.str(); } if (v.type() == nPath) { @@ -2534,7 +2534,7 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v return v1.boolean == v2.boolean; case nString: - return strcmp(v1.string.s, v2.string.s) == 0; + return strcmp(v1.c_str(), v2.c_str()) == 0; case nPath: return strcmp(v1._path, v2._path) == 0; diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index e13d1cb93..20bbd1898 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -113,7 +113,7 @@ static FlakeInput parseFlakeInput(EvalState & state, try { if (attr.name == sUrl) { expectType(state, nString, *attr.value, attr.pos); - url = attr.value->string.s; + url = attr.value->c_str(); attrs.emplace("url", *url); } else if (attr.name == sFlake) { expectType(state, nBool, *attr.value, attr.pos); @@ -122,7 +122,7 @@ static FlakeInput parseFlakeInput(EvalState & state, input.overrides = parseFlakeInputs(state, attr.value, attr.pos, baseDir, lockRootPath, depth + 1); } else if (attr.name == sFollows) { expectType(state, nString, *attr.value, attr.pos); - auto follows(parseInputPath(attr.value->string.s)); + auto follows(parseInputPath(attr.value->c_str())); follows.insert(follows.begin(), lockRootPath.begin(), lockRootPath.end()); input.follows = follows; } else { @@ -131,7 +131,7 @@ static FlakeInput parseFlakeInput(EvalState & state, #pragma GCC diagnostic ignored "-Wswitch-enum" switch (attr.value->type()) { case nString: - attrs.emplace(state.symbols[attr.name], attr.value->string.s); + attrs.emplace(state.symbols[attr.name], attr.value->c_str()); break; case nBool: attrs.emplace(state.symbols[attr.name], Explicit { attr.value->boolean }); @@ -238,7 +238,7 @@ static Flake getFlake( if (auto description = vInfo.attrs->get(state.sDescription)) { expectType(state, nString, *description->value, description->pos); - flake.description = description->value->string.s; + flake.description = description->value->c_str(); } auto sInputs = state.symbols.create("inputs"); diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index e686ffe8c..dadbc3aa3 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -156,7 +156,7 @@ DrvInfo::Outputs DrvInfo::queryOutputs(bool withPaths, bool onlyOutputsToInstall Outputs result; for (auto elem : outTI->listItems()) { if (elem->type() != nString) throw errMsg; - auto out = outputs.find(elem->string.s); + auto out = outputs.find(elem->c_str()); if (out == outputs.end()) throw errMsg; result.insert(*out); } @@ -230,7 +230,7 @@ std::string DrvInfo::queryMetaString(const std::string & name) { Value * v = queryMeta(name); if (!v || v->type() != nString) return ""; - return v->string.s; + return v->c_str(); } @@ -242,7 +242,7 @@ NixInt DrvInfo::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->string.s)) + if (auto n = string2Int(v->c_str())) return *n; } return def; @@ -256,7 +256,7 @@ NixFloat DrvInfo::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->string.s)) + if (auto n = string2Float(v->c_str())) return *n; } return def; @@ -271,8 +271,8 @@ bool DrvInfo::queryMetaBool(const std::string & name, bool def) if (v->type() == nString) { /* Backwards compatibility with before we had support for Boolean meta fields. */ - if (strcmp(v->string.s, "true") == 0) return true; - if (strcmp(v->string.s, "false") == 0) return false; + if (strcmp(v->c_str(), "true") == 0) return true; + if (strcmp(v->c_str(), "false") == 0) return false; } return def; } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 33a2688f1..2d4eb9d33 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -594,7 +594,7 @@ struct CompareValues case nFloat: return v1->fpoint < v2->fpoint; case nString: - return strcmp(v1->string.s, v2->string.s) < 0; + return strcmp(v1->c_str(), v2->c_str()) < 0; case nPath: return strcmp(v1->_path, v2->_path) < 0; case nList: @@ -977,7 +977,7 @@ static void prim_trace(EvalState & state, const PosIdx pos, Value * * args, Valu { state.forceValue(*args[0], pos); if (args[0]->type() == nString) - printError("trace: %1%", args[0]->string.s); + printError("trace: %1%", args[0]->c_str()); else printError("trace: %1%", ValuePrinter(state, *args[0])); if (evalSettings.builtinsTraceDebugger && state.debugRepl && !state.debugTraces.empty()) { @@ -2400,7 +2400,7 @@ static void prim_attrNames(EvalState & state, const PosIdx pos, Value * * args, (v.listElems()[n++] = state.allocValue())->mkString(state.symbols[i.name]); std::sort(v.listElems(), v.listElems() + n, - [](Value * v1, Value * v2) { return strcmp(v1->string.s, v2->string.s) < 0; }); + [](Value * v1, Value * v2) { return strcmp(v1->c_str(), v2->c_str()) < 0; }); } static RegisterPrimOp primop_attrNames({ @@ -2590,7 +2590,7 @@ static void prim_removeAttrs(EvalState & state, const PosIdx pos, Value * * args names.reserve(args[1]->listSize()); for (auto elem : args[1]->listItems()) { state.forceStringNoCtx(*elem, pos, "while evaluating the values of the second argument passed to builtins.removeAttrs"); - names.emplace_back(state.symbols.create(elem->string.s), nullptr); + names.emplace_back(state.symbols.create(elem->c_str()), nullptr); } std::sort(names.begin(), names.end()); @@ -3740,7 +3740,7 @@ static void prim_substring(EvalState & state, const PosIdx pos, Value * * args, if (len == 0) { state.forceValue(*args[2], pos); if (args[2]->type() == nString) { - v.mkString("", args[2]->string.context); + v.mkString("", args[2]->stringContext()); return; } } diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index 3804db6eb..69de2b78b 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -133,7 +133,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg else if (attrName == "toPath") { state.forceValue(*attr.value, attr.pos); - bool isEmptyString = attr.value->type() == nString && attr.value->string.s == std::string(""); + bool isEmptyString = attr.value->type() == nString && attr.value->c_str() == std::string(""); if (isEmptyString) { toPath = StorePathOrGap {}; } diff --git a/src/libexpr/print-ambiguous.cc b/src/libexpr/print-ambiguous.cc index ec30f5073..022358053 100644 --- a/src/libexpr/print-ambiguous.cc +++ b/src/libexpr/print-ambiguous.cc @@ -28,7 +28,7 @@ void printAmbiguous( printLiteralBool(str, v.boolean); break; case nString: - escapeString(str, v.string.s); + escapeString(str, v.c_str()); break; case nPath: str << v.path().to_string(); // !!! escaping? diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 231bde0a0..6b91ed272 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -200,7 +200,7 @@ private: { escapeString( output, - v.string.s, + v.c_str(), { .maxLength = options.maxStringLength, .outputAnsiColors = options.ansiColors, diff --git a/src/libexpr/value-to-json.cc b/src/libexpr/value-to-json.cc index ebb3379a8..73a1bebf6 100644 --- a/src/libexpr/value-to-json.cc +++ b/src/libexpr/value-to-json.cc @@ -32,7 +32,7 @@ json printValueAsJSON(EvalState & state, bool strict, case nString: copyContext(v, context); - out = v.string.s; + out = v.c_str(); break; case nPath: diff --git a/src/libexpr/value-to-xml.cc b/src/libexpr/value-to-xml.cc index 5d1fbd28d..2809aba43 100644 --- a/src/libexpr/value-to-xml.cc +++ b/src/libexpr/value-to-xml.cc @@ -75,7 +75,7 @@ static void printValueAsXML(EvalState & state, bool strict, bool location, case nString: /* !!! show the context? */ copyContext(v, context); - doc.writeEmptyElement("string", singletonAttrs("value", v.string.s)); + doc.writeEmptyElement("string", singletonAttrs("value", v.c_str())); break; case nPath: @@ -97,14 +97,14 @@ static void printValueAsXML(EvalState & state, bool strict, bool location, if (a != v.attrs->end()) { if (strict) state.forceValue(*a->value, a->pos); if (a->value->type() == nString) - xmlAttrs["drvPath"] = drvPath = a->value->string.s; + xmlAttrs["drvPath"] = drvPath = a->value->c_str(); } a = v.attrs->find(state.sOutPath); if (a != v.attrs->end()) { if (strict) state.forceValue(*a->value, a->pos); if (a->value->type() == nString) - xmlAttrs["outPath"] = a->value->string.s; + xmlAttrs["outPath"] = a->value->c_str(); } XMLOpenElement _(doc, "derivation", xmlAttrs); diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 450216ec0..73981d869 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -437,11 +437,35 @@ public: return SourcePath{CanonPath(_path)}; } - std::string_view str() const +// Allow selecting a subset of enum values +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wswitch-enum" + + const char * c_str() const { - assert(internalType == tString); - return std::string_view(string.s); + switch(internalType) { + case tString: return string.s; + default: abort(); + } } + + std::string_view str() + { + switch(internalType) { + case tString: return std::string_view(string.s); + default: abort(); + } + } + + const char * * stringContext() const + { + switch(internalType) { + case tString: return string.context; + default: abort(); + } + } + +#pragma GCC diagnostic pop }; diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index ad255a1e1..a8c1c250b 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -1233,7 +1233,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) else { if (v->type() == nString) { attrs2["type"] = "string"; - attrs2["value"] = v->string.s; + attrs2["value"] = v->c_str(); xml.writeEmptyElement("meta", attrs2); } else if (v->type() == nInt) { attrs2["type"] = "int"; @@ -1253,7 +1253,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) for (auto elem : v->listItems()) { if (elem->type() != nString) continue; XMLAttrs attrs3; - attrs3["value"] = elem->string.s; + attrs3["value"] = elem->c_str(); xml.writeEmptyElement("string", attrs3); } } else if (v->type() == nAttrs) { @@ -1265,7 +1265,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) if(a.value->type() != nString) continue; XMLAttrs attrs3; attrs3["type"] = globals.state->symbols[i.name]; - attrs3["value"] = a.value->string.s; + attrs3["value"] = a.value->c_str(); xml.writeEmptyElement("string", attrs3); } } diff --git a/src/nix/eval.cc b/src/nix/eval.cc index 469ff7391..994a20004 100644 --- a/src/nix/eval.cc +++ b/src/nix/eval.cc @@ -86,7 +86,7 @@ struct CmdEval : MixJSON, InstallableValueCommand, MixReadOnlyOption state->forceValue(v, pos); if (v.type() == nString) // FIXME: disallow strings with contexts? - writeFile(path, v.string.s); + writeFile(path, v.c_str()); else if (v.type() == nAttrs) { if (mkdir(path.c_str(), 0777) == -1) throw SysError("creating directory '%s'", path); diff --git a/tests/unit/libexpr-support/tests/libexpr.hh b/tests/unit/libexpr-support/tests/libexpr.hh index 4055784ca..3d93818d9 100644 --- a/tests/unit/libexpr-support/tests/libexpr.hh +++ b/tests/unit/libexpr-support/tests/libexpr.hh @@ -71,7 +71,7 @@ namespace nix { if (arg.type() != nString) { return false; } - return std::string_view(arg.string.s) == std::string_view(s); + return std::string_view(arg.c_str()) == std::string_view(s); } MATCHER_P(IsIntEq, v, fmt("The string is equal to \"%1%\"", v)) { @@ -107,7 +107,7 @@ namespace nix { *result_listener << "Expected a path got " << arg.type(); return false; } else if (std::string_view(arg._path) != p) { - *result_listener << "Expected a path that equals \"" << p << "\" but got: " << arg.string.s; + *result_listener << "Expected a path that equals \"" << p << "\" but got: " << arg.c_str(); return false; } return true; diff --git a/tests/unit/libexpr/primops.cc b/tests/unit/libexpr/primops.cc index bd174a6c0..ad1f6a260 100644 --- a/tests/unit/libexpr/primops.cc +++ b/tests/unit/libexpr/primops.cc @@ -713,14 +713,14 @@ namespace nix { // FIXME: add a test that verifies the string context is as expected auto v = eval("builtins.replaceStrings [\"oo\" \"a\"] [\"a\" \"i\"] \"foobar\""); ASSERT_EQ(v.type(), nString); - ASSERT_EQ(v.string.s, std::string_view("fabir")); + ASSERT_EQ(v.c_str(), std::string_view("fabir")); } TEST_F(PrimOpTest, concatStringsSep) { // FIXME: add a test that verifies the string context is as expected auto v = eval("builtins.concatStringsSep \"%\" [\"foo\" \"bar\" \"baz\"]"); ASSERT_EQ(v.type(), nString); - ASSERT_EQ(std::string_view(v.string.s), "foo%bar%baz"); + ASSERT_EQ(std::string_view(v.c_str()), "foo%bar%baz"); } TEST_F(PrimOpTest, split1) {