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] 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)