From 9efa32d6515f7d3160cd2ca39c5eb08c64b88ae6 Mon Sep 17 00:00:00 2001 From: piegames Date: Thu, 21 Nov 2024 17:40:09 +0100 Subject: [PATCH] libexpr: Refactor ExprState::pushExpr It has been renamed to `emplaceExpr` to be more consistent with the fact that it actually creates the expression pointer. Moreover, `pushExpr` has been added which directly takes a `unique_ptr`. All manual calls to `exprs.emplace_back` have been removed and replaced with `pushExpr`, and `exprs` is now a protected field. This allows for enforcing invariants and modifying state within `pushExpr`. Change-Id: I995f0bdc1c090cf78080c9f0b7737e2be359b3e4 --- lix/libexpr/parser/grammar.hh | 7 +- lix/libexpr/parser/parser-impl1.inc.cc | 112 +++++++++++++------------ 2 files changed, 64 insertions(+), 55 deletions(-) diff --git a/lix/libexpr/parser/grammar.hh b/lix/libexpr/parser/grammar.hh index 701b40505..e395ffc5c 100644 --- a/lix/libexpr/parser/grammar.hh +++ b/lix/libexpr/parser/grammar.hh @@ -648,10 +648,12 @@ struct nothing : p::nothing { template struct operator_semantics { +public: struct has_attr : grammar::v1::op::has_attr { AttrPathT path; }; +protected: struct OpEntry { OpCtx ctx; uint8_t prec; @@ -688,7 +690,7 @@ struct operator_semantics { // derived class is expected to define members: // - // ExprT applyOp(OpCtx & pos, auto & op, auto &... args); + // void applyOp(OpCtx & pos, auto & op, auto &... args); // [[noreturn]] static void badOperator(OpCtx & pos, auto &... args); void reduce(uint8_t toPrecedence, auto &... args) { @@ -699,12 +701,13 @@ struct operator_semantics { || (kind != grammar::v1::op::kind::leftAssoc && precedence == toPrecedence)) break; std::visit([&, ctx=std::move(ctx)] (auto & op) { - exprs.push_back(static_cast(*this).applyOp(ctx, op, args...)); + static_cast(*this).applyOp(ctx, op, args...); }, op); ops.pop_back(); } } +public: ExprT popExpr() { auto r = std::move(exprs.back()); diff --git a/lix/libexpr/parser/parser-impl1.inc.cc b/lix/libexpr/parser/parser-impl1.inc.cc index 7f8a9bb88..a93cd354f 100644 --- a/lix/libexpr/parser/parser-impl1.inc.cc +++ b/lix/libexpr/parser/parser-impl1.inc.cc @@ -141,38 +141,36 @@ struct ExprState return std::make_unique(pos, state.mkInternalVar(pos, state.s.sub), std::move(args)); } - std::pair> applyOp(PosIdx pos, auto & op, State & state) { + void applyOp(PosIdx pos, auto & op, State & state) { using Op = grammar::v1::op; auto not_ = [] (auto e) { return std::make_unique(std::move(e)); }; - return { - pos, - (overloaded { - [&] (Op::implies) { return applyBinary(pos); }, - [&] (Op::or_) { return applyBinary(pos); }, - [&] (Op::and_) { return applyBinary(pos); }, - [&] (Op::equals) { return applyBinary(pos); }, - [&] (Op::not_equals) { return applyBinary(pos); }, - [&] (Op::less) { return order(pos, true, state); }, - [&] (Op::greater_eq) { return not_(order(pos, true, state)); }, - [&] (Op::greater) { return order(pos, false, state); }, - [&] (Op::less_eq) { return not_(order(pos, false, state)); }, - [&] (Op::update) { return applyBinary(pos); }, - [&] (Op::not_) { return applyUnary(); }, - [&] (Op::plus) { return concatStrings(pos); }, - [&] (Op::minus) { return call(pos, state, state.s.sub); }, - [&] (Op::mul) { return call(pos, state, state.s.mul); }, - [&] (Op::div) { return call(pos, state, state.s.div); }, - [&] (Op::concat) { return applyBinary(pos); }, - [&] (has_attr & a) { return applyUnary(std::move(a.path)); }, - [&] (Op::unary_minus) { return negate(pos, state); }, - [&] (Op::pipe_right) { return pipe(pos, state, true); }, - [&] (Op::pipe_left) { return pipe(pos, state); }, - })(op) - }; + auto expr = (overloaded { + [&] (Op::implies) { return applyBinary(pos); }, + [&] (Op::or_) { return applyBinary(pos); }, + [&] (Op::and_) { return applyBinary(pos); }, + [&] (Op::equals) { return applyBinary(pos); }, + [&] (Op::not_equals) { return applyBinary(pos); }, + [&] (Op::less) { return order(pos, true, state); }, + [&] (Op::greater_eq) { return not_(order(pos, true, state)); }, + [&] (Op::greater) { return order(pos, false, state); }, + [&] (Op::less_eq) { return not_(order(pos, false, state)); }, + [&] (Op::update) { return applyBinary(pos); }, + [&] (Op::not_) { return applyUnary(); }, + [&] (Op::plus) { return concatStrings(pos); }, + [&] (Op::minus) { return call(pos, state, state.s.sub); }, + [&] (Op::mul) { return call(pos, state, state.s.mul); }, + [&] (Op::div) { return call(pos, state, state.s.div); }, + [&] (Op::concat) { return applyBinary(pos); }, + [&] (has_attr & a) { return applyUnary(std::move(a.path)); }, + [&] (Op::unary_minus) { return negate(pos, state); }, + [&] (Op::pipe_right) { return pipe(pos, state, true); }, + [&] (Op::pipe_left) { return pipe(pos, state); }, + })(op); + pushExpr(pos, std::move(expr)); } // always_inline is needed, otherwise pushOp slows down considerably @@ -185,12 +183,19 @@ struct ExprState }); } - template - Expr & pushExpr(PosIdx pos, Args && ... args) + template + inline ExprT & emplaceExpr(PosIdx pos, Args && ... args) { - auto p = std::make_unique(std::forward(args)...); + auto p = std::make_unique(std::forward(args)...); auto & result = *p; - exprs.emplace_back(pos, std::move(p)); + pushExpr(pos, std::move(p)); + return result; + } + + inline Expr & pushExpr(PosIdx pos, std::unique_ptr expr) + { + auto & result = *expr; + exprs.emplace_back(pos, std::move(expr)); return result; } }; @@ -398,9 +403,9 @@ template<> struct BuildAST { template<> struct BuildAST { static void apply(const auto & in, ExprState & s, State & ps) { if (in.string_view() == "__curPos") - s.pushExpr(ps.at(in), ps.at(in)); + s.emplaceExpr(ps.at(in), ps.at(in)); else - s.pushExpr(ps.at(in), ps.at(in), ps.symbols.create(in.string_view())); + s.emplaceExpr(ps.at(in), ps.at(in), ps.symbols.create(in.string_view())); } }; @@ -413,7 +418,7 @@ template<> struct BuildAST { .pos = ps.positions[ps.at(in)], }); } - s.pushExpr(noPos, v); + s.emplaceExpr(noPos, v); } }; @@ -448,7 +453,7 @@ template<> struct BuildAST { }); } }(); - s.pushExpr(noPos, v); + s.emplaceExpr(noPos, v); } }; @@ -542,7 +547,7 @@ template<> struct BuildAST { template<> struct BuildAST : change_head { static void success0(StringState & s, ExprState & e, State &) { - e.exprs.emplace_back(noPos, s.finish()); + e.pushExpr(noPos, s.finish()); } }; @@ -590,7 +595,7 @@ template<> struct BuildAST { template<> struct BuildAST : change_head { static void success(const auto & in, IndStringState & s, ExprState & e, State & ps) { - e.exprs.emplace_back(noPos, ps.stripIndentation(ps.at(in), std::move(s.lines))); + e.pushExpr(noPos, ps.stripIndentation(ps.at(in), std::move(s.lines))); } }; @@ -663,9 +668,9 @@ template<> struct BuildAST : change_head { check_slash(ps.atEnd(in), s, ps); check_slash(ps.atEnd(in), s, ps); if (s.parts.size() == 1) { - e.exprs.emplace_back(noPos, std::move(s.parts.back().second)); + e.pushExpr(noPos, std::move(s.parts.back().second)); } else { - e.pushExpr(ps.at(in), ps.at(in), false, std::move(s.parts)); + e.emplaceExpr(ps.at(in), ps.at(in), false, std::move(s.parts)); } } }; @@ -683,7 +688,7 @@ template<> struct BuildAST { .msg = HintFmt("URL literals are deprecated, allow using them with %s", "--extra-deprecated-features url-literals"), .pos = ps.positions[ps.at(in)] }); - s.pushExpr(ps.at(in), in.string()); + s.emplaceExpr(ps.at(in), in.string()); } }; @@ -701,21 +706,21 @@ template<> struct BuildAST : change_head(pos, pos, std::make_unique(std::move(b.set)), pos, ps.s.body); + s.emplaceExpr(pos, pos, std::make_unique(std::move(b.set)), pos, ps.s.body); } }; template<> struct BuildAST : change_head { static void success(const auto & in, BindingsStateRecSet & b, ExprState & s, State & ps) { b.set.pos = ps.at(in); - s.pushExpr(ps.at(in), std::move(b.set)); + s.emplaceExpr(ps.at(in), std::move(b.set)); } }; template<> struct BuildAST : change_head { static void success(const auto & in, BindingsStateSet & b, ExprState & s, State & ps) { b.set.pos = ps.at(in); - s.pushExpr(ps.at(in), std::move(b.set)); + s.emplaceExpr(ps.at(in), std::move(b.set)); } }; @@ -725,7 +730,7 @@ template<> struct BuildAST : change_head { static void success(const auto & in, ListState & ls, ExprState & s, State & ps) { auto e = std::make_unique(); e->elems = std::move(ls); - s.exprs.emplace_back(ps.at(in), std::move(e)); + s.pushExpr(ps.at(in), std::move(e)); } }; @@ -750,7 +755,7 @@ template<> struct BuildAST { template<> struct BuildAST : change_head { static void success0(AttrState & a, SelectState & s, State &) { - s.e = &s->pushExpr(s.pos, s.pos, s->popExprOnly(), std::move(a.attrs), nullptr); + s.e = &s->emplaceExpr(s.pos, s.pos, s->popExprOnly(), std::move(a.attrs), nullptr); } }; @@ -764,7 +769,7 @@ template<> struct BuildAST { static void apply(const auto & in, SelectState & s, State & ps) { std::vector> args(1); args[0] = std::make_unique(ps.at(in), ps.s.or_); - s->pushExpr(s.pos, s.pos, s->popExprOnly(), std::move(args)); + s->emplaceExpr(s.pos, s.pos, s->popExprOnly(), std::move(args)); } }; @@ -794,11 +799,11 @@ template<> struct BuildAST { // this can happen on occasions such as `

` or `a or b or`, // neither of which are super worth optimizing. s.e->args.push_back(std::move(arg)); - s->exprs.emplace_back(noPos, std::move(fn)); + s->pushExpr(noPos, std::move(fn)); } else { std::vector> args{1}; args[0] = std::move(arg); - s.e = &s->pushExpr(s.pos, s.pos, std::move(fn), std::move(args)); + s.e = &s->emplaceExpr(s.pos, s.pos, std::move(fn), std::move(args)); } } }; @@ -834,21 +839,21 @@ template<> struct BuildAST : change_head static void success(const auto & in, LambdaState & l, ExprState & s, State & ps) { if (l.formals) l.formals = ps.validateFormals(std::move(l.formals), ps.at(in), l.arg); - s.pushExpr(ps.at(in), ps.at(in), l.arg, std::move(l.formals), l->popExprOnly()); + s.emplaceExpr(ps.at(in), ps.at(in), l.arg, std::move(l.formals), l->popExprOnly()); } }; template<> struct BuildAST { static void apply(const auto & in, ExprState & s, State & ps) { auto body = s.popExprOnly(), cond = s.popExprOnly(); - s.pushExpr(ps.at(in), ps.at(in), std::move(cond), std::move(body)); + s.emplaceExpr(ps.at(in), ps.at(in), std::move(cond), std::move(body)); } }; template<> struct BuildAST { static void apply(const auto & in, ExprState & s, State & ps) { auto body = s.popExprOnly(), scope = s.popExprOnly(); - s.pushExpr(ps.at(in), ps.at(in), std::move(scope), std::move(body)); + s.emplaceExpr(ps.at(in), ps.at(in), std::move(scope), std::move(body)); } }; @@ -860,20 +865,21 @@ template<> struct BuildAST : change_headpopExprOnly(); - s.pushExpr(ps.at(in), std::move(b.let)); + s.emplaceExpr(ps.at(in), std::move(b.let)); } }; template<> struct BuildAST { static void apply(const auto & in, ExprState & s, State & ps) { auto else_ = s.popExprOnly(), then = s.popExprOnly(), cond = s.popExprOnly(); - s.pushExpr(ps.at(in), ps.at(in), std::move(cond), std::move(then), std::move(else_)); + s.emplaceExpr(ps.at(in), ps.at(in), std::move(cond), std::move(then), std::move(else_)); } }; template<> struct BuildAST : change_head { static void success0(ExprState & inner, ExprState & outer, State & ps) { - outer.exprs.push_back(inner.finish(ps)); + auto [pos, expr] = inner.finish(ps); + outer.pushExpr(pos, std::move(expr)); } };