From ca9fde1b8827c17d3b12f134f32fa62a11b5401c Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Sat, 1 Nov 2025 13:41:50 -0400
Subject: [PATCH 1/2] In `EvalState::concatLists`, local variable `s` ->
 `strings`

It deserves a better name.

Co-Authored-By: Aspen Smith <root@gws.fyi>
---
 src/libexpr/eval.cc | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index 35a01101b..c2d39d6d5 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -2021,7 +2021,7 @@ void EvalState::concatLists(
 void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
 {
     NixStringContext context;
-    std::vector<BackedStringView> s;
+    std::vector<BackedStringView> strings;
     size_t sSize = 0;
     NixInt n{0};
     NixFloat nf = 0;
@@ -2032,7 +2032,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
     const auto str = [&] {
         std::string result;
         result.reserve(sSize);
-        for (const auto & part : s)
+        for (const auto & part : strings)
             result += *part;
         return result;
     };
@@ -2042,7 +2042,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
     const auto c_str = [&] {
         char * result = allocString(sSize + 1);
         char * tmp = result;
-        for (const auto & part : s) {
+        for (const auto & part : strings) {
             memcpy(tmp, part->data(), part->size());
             tmp += part->size();
         }
@@ -2097,15 +2097,15 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
                     .withFrame(env, *this)
                     .debugThrow();
         } else {
-            if (s.empty())
-                s.reserve(es.size());
+            if (strings.empty())
+                strings.reserve(es.size());
             /* skip canonization of first path, which would only be not
             canonized in the first place if it's coming from a ./${foo} type
             path */
             auto part = state.coerceToString(
                 i_pos, vTmp, context, "while evaluating a path segment", false, firstType == nString, !first);
             sSize += part->size();
-            s.emplace_back(std::move(part));
+            strings.emplace_back(std::move(part));
         }
 
         first = false;

From b67c2f15727350fbd3eaf5d0e86ef9ba62cd65b7 Mon Sep 17 00:00:00 2001
From: Aspen Smith <root@gws.fyi>
Date: Wed, 10 Sep 2025 16:53:21 -0400
Subject: [PATCH 2/2] Inline only-used-once closures in
 `ExprConcatStrings::eval`

Refactor `ExprConcatStrings::eval` by inlining two only-called-once
closures into the call-site, so that the code is easier to reason about
locally (especially since the variables that were closed over were
mutated all over the place within this function).

Also use curly braces with each branch for consistency in the the
resulting code.

This is a pure refactor, but also arguably causes us to depend less on
the optimizer; now, we don't have to make sure that this closure is
inlined.
---
 src/libexpr/eval.cc | 46 +++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index c2d39d6d5..873b88986 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -2029,27 +2029,6 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
     bool first = !forceString;
     ValueType firstType = nString;
 
-    const auto str = [&] {
-        std::string result;
-        result.reserve(sSize);
-        for (const auto & part : strings)
-            result += *part;
-        return result;
-    };
-    /* c_str() is not str().c_str() because we want to create a string
-       Value. allocating a GC'd string directly and moving it into a
-       Value lets us avoid an allocation and copy. */
-    const auto c_str = [&] {
-        char * result = allocString(sSize + 1);
-        char * tmp = result;
-        for (const auto & part : strings) {
-            memcpy(tmp, part->data(), part->size());
-            tmp += part->size();
-        }
-        *tmp = 0;
-        return result;
-    };
-
     // List of returned strings. References to these Values must NOT be persisted.
     SmallTemporaryValueVector<conservativeStackReservation> values(es.size());
     Value * vTmpP = values.data();
@@ -2111,19 +2090,32 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
         first = false;
     }
 
-    if (firstType == nInt)
+    if (firstType == nInt) {
         v.mkInt(n);
-    else if (firstType == nFloat)
+    } else if (firstType == nFloat) {
         v.mkFloat(nf);
-    else if (firstType == nPath) {
+    } else if (firstType == nPath) {
         if (!context.empty())
             state.error<EvalError>("a string that refers to a store path cannot be appended to a path")
                 .atPos(pos)
                 .withFrame(env, *this)
                 .debugThrow();
-        v.mkPath(state.rootPath(CanonPath(str())));
-    } else
-        v.mkStringMove(c_str(), context);
+        std::string result_str;
+        result_str.reserve(sSize);
+        for (const auto & part : strings) {
+            result_str += *part;
+        }
+        v.mkPath(state.rootPath(CanonPath(result_str)));
+    } else {
+        char * result_str = allocString(sSize + 1);
+        char * tmp = result_str;
+        for (const auto & part : strings) {
+            memcpy(tmp, part->data(), part->size());
+            tmp += part->size();
+        }
+        *tmp = 0;
+        v.mkStringMove(result_str, context);
+    }
 }
 
 void ExprPos::eval(EvalState & state, Env & env, Value & v)