From 484290a9e05ea840f9bc6ba3cc98d64ccffe202b Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Fri, 23 Jun 2023 12:01:10 -0400
Subject: [PATCH] Use a struct not `std::pair` for `SearchPathElem`

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.
---
 src/libexpr/eval.hh    |  8 ++++++--
 src/libexpr/parser.y   | 33 ++++++++++++++++++---------------
 src/libexpr/primops.cc |  9 ++++++---
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh
index 8e41bdbd0..7b726a78f 100644
--- a/src/libexpr/eval.hh
+++ b/src/libexpr/eval.hh
@@ -65,8 +65,12 @@ std::string printValue(const EvalState & state, const Value & v);
 std::ostream & operator << (std::ostream & os, const ValueType t);
 
 
-// FIXME: maybe change this to an std::variant<SourcePath, URL>.
-typedef std::pair<std::string, std::string> SearchPathElem;
+struct SearchPathElem
+{
+    std::string prefix;
+    // FIXME: maybe change this to an std::variant<SourcePath, URL>.
+    std::string path;
+};
 typedef std::list<SearchPathElem> SearchPath;
 
 
diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y
index 5c9597f8a..03e6fb02c 100644
--- a/src/libexpr/parser.y
+++ b/src/libexpr/parser.y
@@ -741,7 +741,10 @@ void EvalState::addToSearchPath(const std::string & s)
         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) {
         std::string suffix;
-        if (i.first.empty())
+        if (i.prefix.empty())
             suffix = concatStrings("/", path);
         else {
-            auto s = i.first.size();
-            if (path.compare(0, s, i.first) != 0 ||
+            auto s = i.prefix.size();
+            if (path.compare(0, s, i.prefix) != 0 ||
                 (path.size() > s && path[s] != '/'))
                 continue;
             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)
 {
-    auto i = searchPathResolved.find(elem.second);
+    auto i = searchPathResolved.find(elem.path);
     if (i != searchPathResolved.end()) return i->second;
 
     std::pair<bool, std::string> res;
 
-    if (EvalSettings::isPseudoUrl(elem.second)) {
+    if (EvalSettings::isPseudoUrl(elem.path)) {
         try {
             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) };
         } catch (FileTransferError & e) {
             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, "" };
         }
     }
 
-    else if (hasPrefix(elem.second, "flake:")) {
+    else if (hasPrefix(elem.path, "flake:")) {
         experimentalFeatureSettings.require(Xp::Flakes);
-        auto flakeRef = parseFlakeRef(elem.second.substr(6), {}, true, false);
-        debug("fetching flake search path element '%s''", elem.second);
+        auto flakeRef = parseFlakeRef(elem.path.substr(6), {}, true, false);
+        debug("fetching flake search path element '%s''", elem.path);
         auto storePath = flakeRef.resolve(store).fetchTree(store).first.storePath;
         res = { true, store->toRealPath(storePath) };
     }
 
     else {
-        auto path = absPath(elem.second);
+        auto path = absPath(elem.path);
         if (pathExists(path))
             res = { true, path };
         else {
             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, "" };
         }
     }
 
-    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;
 }
 
diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index 5b2f7e8b7..9dd7bae02 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -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");
@@ -4129,8 +4132,8 @@ void EvalState::createBaseEnv()
     int n = 0;
     for (auto & i : searchPath) {
         auto attrs = buildBindings(2);
-        attrs.alloc("path").mkString(i.second);
-        attrs.alloc("prefix").mkString(i.first);
+        attrs.alloc("path").mkString(i.path);
+        attrs.alloc("prefix").mkString(i.prefix);
         (v.listElems()[n++] = allocValue())->mkAttrs(attrs);
     }
     addConstant("__nixPath", v);