Skip to content

Commit b67c2f1

Browse files
glittersharkEricson2314
authored andcommitted
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.
1 parent ca9fde1 commit b67c2f1

File tree

1 file changed

+19
-27
lines changed

1 file changed

+19
-27
lines changed

src/libexpr/eval.cc

Lines changed: 19 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2029,27 +2029,6 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
20292029
bool first = !forceString;
20302030
ValueType firstType = nString;
20312031

2032-
const auto str = [&] {
2033-
std::string result;
2034-
result.reserve(sSize);
2035-
for (const auto & part : strings)
2036-
result += *part;
2037-
return result;
2038-
};
2039-
/* c_str() is not str().c_str() because we want to create a string
2040-
Value. allocating a GC'd string directly and moving it into a
2041-
Value lets us avoid an allocation and copy. */
2042-
const auto c_str = [&] {
2043-
char * result = allocString(sSize + 1);
2044-
char * tmp = result;
2045-
for (const auto & part : strings) {
2046-
memcpy(tmp, part->data(), part->size());
2047-
tmp += part->size();
2048-
}
2049-
*tmp = 0;
2050-
return result;
2051-
};
2052-
20532032
// List of returned strings. References to these Values must NOT be persisted.
20542033
SmallTemporaryValueVector<conservativeStackReservation> values(es.size());
20552034
Value * vTmpP = values.data();
@@ -2111,19 +2090,32 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
21112090
first = false;
21122091
}
21132092

2114-
if (firstType == nInt)
2093+
if (firstType == nInt) {
21152094
v.mkInt(n);
2116-
else if (firstType == nFloat)
2095+
} else if (firstType == nFloat) {
21172096
v.mkFloat(nf);
2118-
else if (firstType == nPath) {
2097+
} else if (firstType == nPath) {
21192098
if (!context.empty())
21202099
state.error<EvalError>("a string that refers to a store path cannot be appended to a path")
21212100
.atPos(pos)
21222101
.withFrame(env, *this)
21232102
.debugThrow();
2124-
v.mkPath(state.rootPath(CanonPath(str())));
2125-
} else
2126-
v.mkStringMove(c_str(), context);
2103+
std::string result_str;
2104+
result_str.reserve(sSize);
2105+
for (const auto & part : strings) {
2106+
result_str += *part;
2107+
}
2108+
v.mkPath(state.rootPath(CanonPath(result_str)));
2109+
} else {
2110+
char * result_str = allocString(sSize + 1);
2111+
char * tmp = result_str;
2112+
for (const auto & part : strings) {
2113+
memcpy(tmp, part->data(), part->size());
2114+
tmp += part->size();
2115+
}
2116+
*tmp = 0;
2117+
v.mkStringMove(result_str, context);
2118+
}
21272119
}
21282120

21292121
void ExprPos::eval(EvalState & state, Env & env, Value & v)

0 commit comments

Comments
 (0)