lix-project/lix
20
55
Fork 31
Code Issues 344 Code Review (Gerrit) Projects 3 Releases Wiki Activity

Use a struct not std::pair for SearchPathElem

Browse source 
I got very confused trying to keep all the `first` and `second` straight
reading the code, *especially* as there is also another `(boolean,
string)` pair type also being used.

Named fields is much better.

There are other cleanups that we can do (for example, the existing
TODO), but we can do them later. Doing them now would just make this
harder to review.
This commit is contained in:
John Ericson 2023-06-23 12:01:10 -04:00
parent fd4f03b8fd
commit 484290a9e0
3 changed files with 30 additions and 20 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
src/libexpr/eval.hh
View file

@ -65,8 +65,12 @@ std::string printValue(const EvalState & state, const Value & v);
std::ostream & operator << (std::ostream & os, const ValueType t); std::ostream & operator << (std::ostream & os, const ValueType t);
// FIXME: maybe change this to an std::variant<SourcePath, URL>. struct SearchPathElem
typedef std::pair<std::string, std::string> SearchPathElem; {
std::string prefix;
// FIXME: maybe change this to an std::variant<SourcePath, URL>.
std::string path;
};
typedef std::list<SearchPathElem> SearchPath; typedef std::list<SearchPathElem> SearchPath;

33
src/libexpr/parser.y
View file

@ -741,7 +741,10 @@ void EvalState::addToSearchPath(const std::string & s)
path = std::string(s, pos + 1); path = std::string(s, pos + 1);
} }
searchPath.emplace_back(prefix, path); searchPath.emplace_back(SearchPathElem {
.prefix = prefix,
.path = path,
});
} }
@ -755,11 +758,11 @@ SourcePath EvalState::findFile(SearchPath & searchPath, const std::string_view p
{ {
for (auto & i : searchPath) { for (auto & i : searchPath) {
std::string suffix; std::string suffix;
if (i.first.empty()) if (i.prefix.empty())
suffix = concatStrings("/", path); suffix = concatStrings("/", path);
else { else {
auto s = i.first.size(); auto s = i.prefix.size();
if (path.compare(0, s, i.first) != 0 || if (path.compare(0, s, i.prefix) != 0 ||
(path.size() > s && path[s] != '/')) (path.size() > s && path[s] != '/'))
continue; continue;
suffix = path.size() == s ? "" : concatStrings("/", path.substr(s)); suffix = path.size() == s ? "" : concatStrings("/", path.substr(s));
@ -785,47 +788,47 @@ SourcePath EvalState::findFile(SearchPath & searchPath, const std::string_view p
std::pair<bool, std::string> EvalState::resolveSearchPathElem(const SearchPathElem & elem) std::pair<bool, std::string> EvalState::resolveSearchPathElem(const SearchPathElem & elem)
{ {
auto i = searchPathResolved.find(elem.second); auto i = searchPathResolved.find(elem.path);
if (i != searchPathResolved.end()) return i->second; if (i != searchPathResolved.end()) return i->second;
std::pair<bool, std::string> res; std::pair<bool, std::string> res;
if (EvalSettings::isPseudoUrl(elem.second)) { if (EvalSettings::isPseudoUrl(elem.path)) {
try { try {
auto storePath = fetchers::downloadTarball( auto storePath = fetchers::downloadTarball(
store, EvalSettings::resolvePseudoUrl(elem.second), "source", false).tree.storePath; store, EvalSettings::resolvePseudoUrl(elem.path), "source", false).tree.storePath;
res = { true, store->toRealPath(storePath) }; res = { true, store->toRealPath(storePath) };
} catch (FileTransferError & e) { } catch (FileTransferError & e) {
logWarning({ logWarning({
.msg = hintfmt("Nix search path entry '%1%' cannot be downloaded, ignoring", elem.second) .msg = hintfmt("Nix search path entry '%1%' cannot be downloaded, ignoring", elem.path)
}); });
res = { false, "" }; res = { false, "" };
} }
} }
else if (hasPrefix(elem.second, "flake:")) { else if (hasPrefix(elem.path, "flake:")) {
experimentalFeatureSettings.require(Xp::Flakes); experimentalFeatureSettings.require(Xp::Flakes);
auto flakeRef = parseFlakeRef(elem.second.substr(6), {}, true, false); auto flakeRef = parseFlakeRef(elem.path.substr(6), {}, true, false);
debug("fetching flake search path element '%s''", elem.second); debug("fetching flake search path element '%s''", elem.path);
auto storePath = flakeRef.resolve(store).fetchTree(store).first.storePath; auto storePath = flakeRef.resolve(store).fetchTree(store).first.storePath;
res = { true, store->toRealPath(storePath) }; res = { true, store->toRealPath(storePath) };
} }
else { else {
auto path = absPath(elem.second); auto path = absPath(elem.path);
if (pathExists(path)) if (pathExists(path))
res = { true, path }; res = { true, path };
else { else {
logWarning({ logWarning({
.msg = hintfmt("Nix search path entry '%1%' does not exist, ignoring", elem.second) .msg = hintfmt("Nix search path entry '%1%' does not exist, ignoring", elem.path)
}); });
res = { false, "" }; res = { false, "" };
} }
} }
debug("resolved search path element '%s' to '%s'", elem.second, res.second); debug("resolved search path element '%s' to '%s'", elem.path, res.second);
searchPathResolved[elem.second] = res; searchPathResolved[elem.path] = res;
return res; return res;
} }

9
src/libexpr/primops.cc
View file

@ -1656,7 +1656,10 @@ static void prim_findFile(EvalState & state, const PosIdx pos, Value * * args, V
})); }));
} }
searchPath.emplace_back(prefix, path); searchPath.emplace_back(SearchPathElem {
.prefix = prefix,
.path = path,
});
} }
auto path = state.forceStringNoCtx(*args[1], pos, "while evaluating the second argument passed to builtins.findFile"); auto path = state.forceStringNoCtx(*args[1], pos, "while evaluating the second argument passed to builtins.findFile");
@ -4129,8 +4132,8 @@ void EvalState::createBaseEnv()
int n = 0; int n = 0;
for (auto & i : searchPath) { for (auto & i : searchPath) {
auto attrs = buildBindings(2); auto attrs = buildBindings(2);
attrs.alloc("path").mkString(i.second); attrs.alloc("path").mkString(i.path);
attrs.alloc("prefix").mkString(i.first); attrs.alloc("prefix").mkString(i.prefix);
(v.listElems()[n++] = allocValue())->mkAttrs(attrs); (v.listElems()[n++] = allocValue())->mkAttrs(attrs);
} }
addConstant("__nixPath", v); addConstant("__nixPath", v);