From f86eafa3b6dea7992b2204c6106fcf5e2995ca3a Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sat, 16 Mar 2024 15:18:17 +0100 Subject: [PATCH] store ExprConcatStrings elements as direct vector storing a pointer only adds an unnecessary indirection and memory allocation. Change-Id: If06dd05effdf1ccb0df0873580f50c775608925d --- src/libexpr/eval.cc | 6 +++--- src/libexpr/nixexpr.cc | 4 ++-- src/libexpr/nixexpr.hh | 8 ++++---- src/libexpr/parser-state.hh | 14 ++++++-------- src/libexpr/parser.y | 10 +++++++--- 5 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 96a0eb7f5..f600ddad6 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2010,10 +2010,10 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) }; // List of returned strings. References to these Values must NOT be persisted. - SmallTemporaryValueVector values(es->size()); + SmallTemporaryValueVector values(es.size()); Value * vTmpP = values.data(); - for (auto & [i_pos, i] : *es) { + for (auto & [i_pos, i] : es) { Value & vTmp = *vTmpP++; i->eval(state, env, vTmp); @@ -2043,7 +2043,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) } else state.error("cannot add %1% to a float", showType(vTmp)).atPos(i_pos).withFrame(env, *this).debugThrow(); } else { - if (s.empty()) s.reserve(es->size()); + if (s.empty()) s.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 */ diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index a9989b59e..9044309ef 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -230,7 +230,7 @@ void ExprConcatStrings::show(const SymbolTable & symbols, std::ostream & str) co { bool first = true; str << "("; - for (auto & i : *es) { + for (auto & i : es) { if (first) first = false; else str << " + "; i.second->show(symbols, str); } @@ -546,7 +546,7 @@ void ExprConcatStrings::bindVars(EvalState & es, const std::shared_ptres) + for (auto & i : this->es) i.second->bindVars(es, env); } diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index e5755bb99..c9c0d9924 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -296,7 +296,7 @@ struct ExprCall : Expr std::vector args; PosIdx pos; ExprCall(const PosIdx & pos, Expr * fun, std::vector && args) - : fun(fun), args(args), pos(pos) + : fun(fun), args(std::move(args)), pos(pos) { } PosIdx getPos() const override { return pos; } COMMON_METHODS @@ -378,9 +378,9 @@ struct ExprConcatStrings : Expr { PosIdx pos; bool forceString; - std::vector> * es; - ExprConcatStrings(const PosIdx & pos, bool forceString, std::vector> * es) - : pos(pos), forceString(forceString), es(es) { }; + std::vector> es; + ExprConcatStrings(const PosIdx & pos, bool forceString, std::vector> es) + : pos(pos), forceString(forceString), es(std::move(es)) { }; PosIdx getPos() const override { return pos; } COMMON_METHODS }; diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index c96e64156..db5ca02c9 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -216,7 +216,7 @@ inline Expr * ParserState::stripIndentation(const PosIdx pos, } /* Strip spaces from each line. */ - auto * es2 = new std::vector>; + std::vector> es2; atStartOfLine = true; size_t curDropped = 0; size_t n = es.size(); @@ -224,7 +224,7 @@ inline Expr * ParserState::stripIndentation(const PosIdx pos, const auto trimExpr = [&] (Expr * e) { atStartOfLine = false; curDropped = 0; - es2->emplace_back(i->first, e); + es2.emplace_back(i->first, e); }; const auto trimString = [&] (const StringToken & t) { std::string s2; @@ -256,19 +256,17 @@ inline Expr * ParserState::stripIndentation(const PosIdx pos, s2 = std::string(s2, 0, p + 1); } - es2->emplace_back(i->first, new ExprString(std::move(s2))); + es2.emplace_back(i->first, new ExprString(std::move(s2))); }; for (; i != es.end(); ++i, --n) { std::visit(overloaded { trimExpr, trimString }, i->second); } /* If this is a single string, then don't do a concatenation. */ - if (es2->size() == 1 && dynamic_cast((*es2)[0].second)) { - auto *const result = (*es2)[0].second; - delete es2; - return result; + if (es2.size() == 1 && dynamic_cast(es2[0].second)) { + return es2[0].second; } - return new ExprConcatStrings(pos, true, es2); + return new ExprConcatStrings(pos, true, std::move(es2)); } inline PosIdx ParserState::at(const ParserLocation & loc) diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index dbfeb38d9..458916d1c 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -209,7 +209,7 @@ expr_op | expr_op UPDATE expr_op { $$ = new ExprOpUpdate(state->at(@2), $1, $3); } | expr_op '?' attrpath { $$ = new ExprOpHasAttr($1, std::move(*$3)); delete $3; } | expr_op '+' expr_op - { $$ = new ExprConcatStrings(state->at(@2), false, new std::vector >({{state->at(@1), $1}, {state->at(@3), $3}})); } + { $$ = new ExprConcatStrings(state->at(@2), false, {{state->at(@1), $1}, {state->at(@3), $3}}); } | expr_op '-' expr_op { $$ = new ExprCall(state->at(@2), new ExprVar(state->s.sub), {$1, $3}); } | expr_op '*' expr_op { $$ = new ExprCall(state->at(@2), new ExprVar(state->s.mul), {$1, $3}); } | expr_op '/' expr_op { $$ = new ExprCall(state->at(@2), new ExprVar(state->s.div), {$1, $3}); } @@ -258,7 +258,8 @@ expr_simple | path_start PATH_END | path_start string_parts_interpolated PATH_END { $2->insert($2->begin(), {state->at(@1), $1}); - $$ = new ExprConcatStrings(CUR_POS, false, $2); + $$ = new ExprConcatStrings(CUR_POS, false, std::move(*$2)); + delete $2; } | SPATH { std::string path($1.p + 1, $1.l - 2); @@ -290,7 +291,10 @@ expr_simple string_parts : STR { $$ = new ExprString(std::string($1)); } - | string_parts_interpolated { $$ = new ExprConcatStrings(CUR_POS, true, $1); } + | string_parts_interpolated + { $$ = new ExprConcatStrings(CUR_POS, true, std::move(*$1)); + delete $1; + } | { $$ = new ExprString(""); } ;