From 494624d6c397c48932121fdff4f630a9b7f6b278 Mon Sep 17 00:00:00 2001 From: Ben Deane Date: Sun, 2 Nov 2025 10:07:05 -0700 Subject: [PATCH] :art: Refactor flow builder Problem: - The flow `service`s and `builder`s are too coupled to their implementations, viz: - `flow::FunctionPtr` is used for both the interface function on the `service` and the node type for the inlined function list; these have nothing to do with each other. - The `service` needs to know about the builder's interface. - The `graph_builder` knows about details of inlining the function list. - The `impl` is poorly named (it was named for the first, but now not the only, implementation). Solution: - Inject the builders and implementations at the correct levels. - Instead of passing through template arguments, expose names and interfaces from lower levels. This enables concepts (TBD). --- CMakeLists.txt | 12 +++-- include/flow/builder.hpp | 53 +++++++++++++-------- include/flow/common.hpp | 5 -- include/flow/{detail => dsl}/par.hpp | 6 +-- include/flow/{detail => dsl}/seq.hpp | 6 +-- include/flow/dsl/subgraph_identity.hpp | 5 ++ include/flow/{detail => dsl}/walk.hpp | 2 +- include/flow/flow.hpp | 8 ++-- include/flow/{impl.hpp => func_list.hpp} | 50 ++++++++------------ include/flow/graph_builder.hpp | 59 ++++++------------------ include/flow/graphviz_builder.hpp | 16 +++---- include/flow/log.hpp | 10 ++++ include/flow/run.hpp | 3 +- include/flow/service.hpp | 26 +++++++++++ include/flow/step.hpp | 16 +++---- include/flow/subgraph_identity.hpp | 5 -- include/seq/builder.hpp | 8 ++-- include/seq/step.hpp | 4 +- test/flow/CMakeLists.txt | 2 +- test/flow/{graph.cpp => builder.cpp} | 24 +++++----- test/flow/flow.cpp | 13 ++---- test/flow/flow_uninit.cpp | 2 +- test/flow/graph_builder.cpp | 12 ++--- test/seq/sequencer.cpp | 3 +- usage_test/main_flow.cpp | 12 +++-- 25 files changed, 181 insertions(+), 181 deletions(-) delete mode 100644 include/flow/common.hpp rename include/flow/{detail => dsl}/par.hpp (94%) rename include/flow/{detail => dsl}/seq.hpp (95%) create mode 100644 include/flow/dsl/subgraph_identity.hpp rename include/flow/{detail => dsl}/walk.hpp (98%) rename include/flow/{impl.hpp => func_list.hpp} (75%) create mode 100644 include/flow/service.hpp delete mode 100644 include/flow/subgraph_identity.hpp rename test/flow/{graph.cpp => builder.cpp} (55%) diff --git a/CMakeLists.txt b/CMakeLists.txt index f9bb43fb..fa240ea9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -241,15 +241,17 @@ target_sources( include FILES include/flow/builder.hpp - include/flow/common.hpp - include/flow/detail/par.hpp - include/flow/detail/seq.hpp - include/flow/detail/walk.hpp + include/flow/dsl/par.hpp + include/flow/dsl/seq.hpp + include/flow/dsl/subgraph_identity.hpp + include/flow/dsl/walk.hpp include/flow/flow.hpp + include/flow/func_list.hpp include/flow/graph_builder.hpp include/flow/graphviz_builder.hpp - include/flow/impl.hpp + include/flow/log.hpp include/flow/run.hpp + include/flow/service.hpp include/flow/step.hpp) add_library(cib_seq INTERFACE) diff --git a/include/flow/builder.hpp b/include/flow/builder.hpp index 67908482..a6ba8053 100644 --- a/include/flow/builder.hpp +++ b/include/flow/builder.hpp @@ -1,30 +1,47 @@ #pragma once -#include +#include #include -#include #include -#include #include -#include +#include +#include +#include + +#include namespace flow { -template > -using builder = graph; +template +class builder_for { + template + friend constexpr auto tag_invoke(Tag, builder_for const &b) { + return b.fragments.apply([](auto const &...frags) { + return stdx::tuple_cat(Tag{}(frags)...); + }); + } -template > -struct service { - using builder_t = builder; - using interface_t = FunctionPtr; - - CONSTEVAL static auto uninitialized() -> interface_t { - return [] { - using namespace stdx::literals; - stdx::panic<"Attempting to run flow ("_cts + Name + - ") before it is initialized"_cts>(); - }; + public: + using interface_t = typename Renderer::interface_t; + constexpr static auto name = Renderer::name; + + template + [[nodiscard]] constexpr auto add(Ns &&...ns) { + return fragments.apply([&](auto &...frags) { + return builder_for...>{ + {frags..., std::forward(ns)...}}; + }); } + + template + [[nodiscard]] constexpr static auto build() { + return Renderer::template render(); + } + + stdx::tuple fragments; }; + +template > +using builder = builder_for>; } // namespace flow diff --git a/include/flow/common.hpp b/include/flow/common.hpp deleted file mode 100644 index e10de213..00000000 --- a/include/flow/common.hpp +++ /dev/null @@ -1,5 +0,0 @@ -#pragma once - -namespace flow { -using FunctionPtr = auto (*)() -> void; -} // namespace flow diff --git a/include/flow/detail/par.hpp b/include/flow/dsl/par.hpp similarity index 94% rename from include/flow/detail/par.hpp rename to include/flow/dsl/par.hpp index 17912667..353dcc99 100644 --- a/include/flow/detail/par.hpp +++ b/include/flow/dsl/par.hpp @@ -1,7 +1,7 @@ #pragma once -#include -#include +#include +#include #include @@ -59,7 +59,7 @@ template } template + flow::dsl::subgraph_identity Identity> constexpr auto make_runtime_conditional(Cond, flow::dsl::par) { auto lhs = make_runtime_conditional(Cond{}, Lhs{}); diff --git a/include/flow/detail/seq.hpp b/include/flow/dsl/seq.hpp similarity index 95% rename from include/flow/detail/seq.hpp rename to include/flow/dsl/seq.hpp index 6494d280..b2d45c1d 100644 --- a/include/flow/detail/seq.hpp +++ b/include/flow/dsl/seq.hpp @@ -1,8 +1,8 @@ #pragma once #include -#include -#include +#include +#include #include @@ -72,7 +72,7 @@ template } template + flow::dsl::subgraph_identity Identity, typename EdgeCond> constexpr auto make_runtime_conditional(Cond, flow::dsl::seq) { auto lhs = make_runtime_conditional(Cond{}, Lhs{}); diff --git a/include/flow/dsl/subgraph_identity.hpp b/include/flow/dsl/subgraph_identity.hpp new file mode 100644 index 00000000..5586b7ad --- /dev/null +++ b/include/flow/dsl/subgraph_identity.hpp @@ -0,0 +1,5 @@ +#pragma once + +namespace flow::dsl { +enum struct subgraph_identity : bool { VALUE, REFERENCE }; +} diff --git a/include/flow/detail/walk.hpp b/include/flow/dsl/walk.hpp similarity index 98% rename from include/flow/detail/walk.hpp rename to include/flow/dsl/walk.hpp index 596158b8..7887fd4d 100644 --- a/include/flow/detail/walk.hpp +++ b/include/flow/dsl/walk.hpp @@ -1,6 +1,6 @@ #pragma once -#include +#include #include #include diff --git a/include/flow/flow.hpp b/include/flow/flow.hpp index 57e95f27..2d41cd0b 100644 --- a/include/flow/flow.hpp +++ b/include/flow/flow.hpp @@ -1,9 +1,7 @@ #pragma once -#include -#include -#include -#include -#include +#include +#include #include +#include #include diff --git a/include/flow/impl.hpp b/include/flow/func_list.hpp similarity index 75% rename from include/flow/impl.hpp rename to include/flow/func_list.hpp index 05f6af31..546d7bbd 100644 --- a/include/flow/impl.hpp +++ b/include/flow/func_list.hpp @@ -1,6 +1,5 @@ #pragma once -#include #include #include @@ -11,18 +10,10 @@ #include #include -#include #include #include namespace flow { -template -concept log_policy = requires { - { - T::template log(stdx::ct_format<"">()) - } -> std::same_as; -}; - namespace detail { template constexpr static auto run_func = []() -> void { @@ -35,28 +26,7 @@ constexpr static auto run_func = []() -> void { typename CTNode::func_t{}(); } }; -} // namespace detail - -template -struct impl { - using node_t = FunctionPtr; - std::array functionPtrs{}; - - constexpr static auto name = Name; - - template - constexpr static auto create_node(CTNode) -> node_t { - constexpr auto fp = detail::run_func; - return fp; - } - - constexpr explicit(true) impl(stdx::span steps) { - std::copy(std::cbegin(steps), std::cend(steps), - std::begin(functionPtrs)); - } -}; -namespace detail { template struct inlined_func_list { constexpr static auto active = sizeof...(FuncPtrs) > 0; @@ -75,4 +45,24 @@ struct inlined_func_list { } }; } // namespace detail + +template +struct func_list { + using node_t = auto (*)() -> void; + std::array nodes{}; + + template + constexpr static auto create_node(CTNode) -> node_t { + constexpr auto fp = detail::run_func; + return fp; + } + + constexpr explicit(true) + func_list(stdx::span steps) { + std::copy(std::cbegin(steps), std::cend(steps), std::begin(nodes)); + } + + template + using finalized_t = detail::inlined_func_list; +}; } // namespace flow diff --git a/include/flow/graph_builder.hpp b/include/flow/graph_builder.hpp index 490628a8..d4073679 100644 --- a/include/flow/graph_builder.hpp +++ b/include/flow/graph_builder.hpp @@ -1,8 +1,7 @@ #pragma once -#include -#include -#include +#include +#include #include #include @@ -58,9 +57,9 @@ template using name_for = typename T::name_t; }); } -template typename Impl, - typename LogPolicy = log_policy_t> +template , + template typename Impl = + func_list> struct graph_builder { // NOLINTBEGIN(readability-function-cognitive-complexity) template @@ -234,6 +233,9 @@ struct graph_builder { return topo_sort(g); } + constexpr static auto name = Name; + using interface_t = auto (*)() -> void; + template class built_flow { constexpr static auto built() { constexpr auto v = Initialized::value; @@ -241,22 +243,19 @@ struct graph_builder { static_assert(built.has_value(), "Topological sort failed: cycle in flow"); - constexpr auto functionPtrs = built->functionPtrs; - constexpr auto size = std::size(functionPtrs); - constexpr auto name = built->name; - + using impl_t = typename decltype(built)::value_type; + constexpr auto nodes = built->nodes; return [&](std::index_sequence) { - return detail::inlined_func_list{}; - }(std::make_index_sequence{}); + return typename impl_t::template finalized_t{}; + }(std::make_index_sequence{}); } constexpr static auto run() { built()(); } public: // NOLINTNEXTLINE(google-explicit-constructor) - constexpr explicit(false) operator FunctionPtr() const { return run; } - constexpr auto operator()() const -> void { run(); } + constexpr explicit(false) operator interface_t() const { return run; } + constexpr auto operator()() const { return run(); } constexpr static bool active = decltype(built())::active; }; @@ -265,34 +264,4 @@ struct graph_builder { return {}; } }; - -template , - typename Renderer = graph_builder, - flow::dsl::subgraph... Fragments> -class graph { - template - friend constexpr auto tag_invoke(Tag, graph const &g) { - return g.fragments.apply([](auto const &...frags) { - return stdx::tuple_cat(Tag{}(frags)...); - }); - } - - public: - template - [[nodiscard]] constexpr auto add(Ns &&...ns) { - return fragments.apply([&](auto &...frags) { - return graph...>{ - {frags..., std::forward(ns)...}}; - }); - } - - template - [[nodiscard]] constexpr static auto build() { - return Renderer::template render(); - } - - constexpr static auto name = Name; - stdx::tuple fragments; -}; } // namespace flow diff --git a/include/flow/graphviz_builder.hpp b/include/flow/graphviz_builder.hpp index 96b524d3..0ffe29d7 100644 --- a/include/flow/graphviz_builder.hpp +++ b/include/flow/graphviz_builder.hpp @@ -1,7 +1,8 @@ #pragma once -#include +#include +#include #include #include @@ -9,9 +10,7 @@ #include namespace flow { -using VizFunctionPtr = auto (*)() -> std::string; - -struct graphviz_builder { +template struct graphviz_builder { template [[nodiscard]] static auto build(Graph const &input) { auto const nodes = flow::dsl::get_nodes(input); @@ -54,6 +53,9 @@ struct graphviz_builder { return output; } + constexpr static auto name = Name; + using interface_t = auto (*)() -> std::string; + template class built_flow { static auto built() { auto const v = Initialized::value; @@ -64,10 +66,8 @@ struct graphviz_builder { public: // NOLINTNEXTLINE(google-explicit-constructor) - constexpr explicit(false) operator VizFunctionPtr() const { - return run; - } - auto operator()() const -> std::string { return run(); } + constexpr explicit(false) operator interface_t() const { return run; } + auto operator()() const { return run(); } constexpr static bool active = true; }; diff --git a/include/flow/log.hpp b/include/flow/log.hpp index aca46249..432090eb 100644 --- a/include/flow/log.hpp +++ b/include/flow/log.hpp @@ -4,11 +4,14 @@ #include #include +#include #include #include #include +#include #include +#include namespace flow { using default_log_env = @@ -52,4 +55,11 @@ struct normal { template using log_policy_t = stdx::conditional_t; + +template +concept log_policy = requires { + { + T::template log(stdx::ct_format<"">()) + } -> std::same_as; +}; } // namespace flow diff --git a/include/flow/run.hpp b/include/flow/run.hpp index 92aa8ef0..0a3dc8ef 100644 --- a/include/flow/run.hpp +++ b/include/flow/run.hpp @@ -1,7 +1,6 @@ #pragma once #include -#include namespace flow { /** @@ -10,5 +9,5 @@ namespace flow { * @tparam Tag Type of the flow to be ran. This is the name of the flow::builder * used to declare and build the flow. */ -template FunctionPtr &run = cib::service; +template auto &run = cib::service; } // namespace flow diff --git a/include/flow/service.hpp b/include/flow/service.hpp new file mode 100644 index 00000000..087a46d9 --- /dev/null +++ b/include/flow/service.hpp @@ -0,0 +1,26 @@ +#pragma once + +#include +#include + +#include +#include +#include + +namespace flow { +template struct service_for { + using builder_t = Builder; + using interface_t = typename builder_t::interface_t; + + CONSTEVAL static auto uninitialized() -> interface_t { + return [] { + using namespace stdx::literals; + stdx::panic<"Attempting to run flow ("_cts + builder_t::name + + ") before it is initialized"_cts>(); + }; + } +}; + +template > +using service = service_for>; +} // namespace flow diff --git a/include/flow/step.hpp b/include/flow/step.hpp index c4933039..7e7f3261 100644 --- a/include/flow/step.hpp +++ b/include/flow/step.hpp @@ -2,9 +2,8 @@ #include #include -#include +#include #include -#include #include #include @@ -14,7 +13,7 @@ namespace flow { template + dsl::subgraph_identity Identity, typename Cond, typename F> struct ct_node { using is_subgraph = void; using name_t = stdx::cts_t; @@ -26,8 +25,9 @@ struct ct_node { constexpr static auto condition = Cond{}; constexpr auto operator*() const { - if constexpr (Identity == subgraph_identity::REFERENCE) { - return ct_node{}; + if constexpr (Identity == dsl::subgraph_identity::REFERENCE) { + return ct_node{}; } else { return ct_node{}; } @@ -37,7 +37,7 @@ struct ct_node { namespace detail { template [[nodiscard]] constexpr auto make_node() { - return ct_node{}; } @@ -78,10 +78,10 @@ template } // namespace literals template + dsl::subgraph_identity Identity, typename NodeCond, typename F> constexpr auto make_runtime_conditional(Cond, ct_node) { - if constexpr (Identity == subgraph_identity::VALUE) { + if constexpr (Identity == dsl::subgraph_identity::VALUE) { return ct_node{}; } else { diff --git a/include/flow/subgraph_identity.hpp b/include/flow/subgraph_identity.hpp deleted file mode 100644 index cf974aa9..00000000 --- a/include/flow/subgraph_identity.hpp +++ /dev/null @@ -1,5 +0,0 @@ -#pragma once - -namespace flow { -enum class subgraph_identity { VALUE, REFERENCE }; -} diff --git a/include/seq/builder.hpp b/include/seq/builder.hpp index fc6dcf2d..a1968782 100644 --- a/include/seq/builder.hpp +++ b/include/seq/builder.hpp @@ -1,7 +1,6 @@ #pragma once -#include -#include +#include #include #include @@ -10,13 +9,12 @@ namespace seq { template > -using builder = - flow::graph>; +using builder = flow::builder_for>; template > struct service { using builder_t = builder; - using interface_t = flow::FunctionPtr; + using interface_t = void (*)(); }; } // namespace seq diff --git a/include/seq/step.hpp b/include/seq/step.hpp index 2911f89e..d8fb81a8 100644 --- a/include/seq/step.hpp +++ b/include/seq/step.hpp @@ -1,7 +1,7 @@ #pragma once #include -#include +#include #include #include @@ -32,7 +32,7 @@ struct ct_step : rt_step { using is_subgraph = void; using name_t = stdx::cts_t; - constexpr static auto identity = flow::subgraph_identity::VALUE; + constexpr static auto identity = flow::dsl::subgraph_identity::VALUE; constexpr static auto condition = cib::detail::always_condition; diff --git a/test/flow/CMakeLists.txt b/test/flow/CMakeLists.txt index d95f5569..028b10dd 100644 --- a/test/flow/CMakeLists.txt +++ b/test/flow/CMakeLists.txt @@ -10,9 +10,9 @@ add_unit_test( add_tests( FILES + builder flow flow_uninit - graph graph_builder logging log_levels diff --git a/test/flow/graph.cpp b/test/flow/builder.cpp similarity index 55% rename from test/flow/graph.cpp rename to test/flow/builder.cpp index 758082f2..9808cc06 100644 --- a/test/flow/graph.cpp +++ b/test/flow/builder.cpp @@ -20,32 +20,32 @@ constexpr auto b = flow::action<"b">([] {}); constexpr auto c = flow::action<"c">([] {}); } // namespace -TEST_CASE("node size (empty graph)", "[graph]") { - constexpr auto g = flow::graph<>{}; +TEST_CASE("node size (empty graph)", "[builder]") { + constexpr auto g = flow::builder<>{}; STATIC_REQUIRE(node_size(g) == 0); } -TEST_CASE("node size (single action)", "[graph]") { - constexpr auto g = flow::graph<>{}.add(*a >> *b); +TEST_CASE("node size (single action)", "[builder]") { + constexpr auto g = flow::builder<>{}.add(*a >> *b); STATIC_REQUIRE(node_size(g) == 2); } -TEST_CASE("node size (overlapping actions)", "[graph]") { - constexpr auto g = flow::graph<>{}.add(*a >> *b).add(b >> *c); +TEST_CASE("node size (overlapping actions)", "[builder]") { + constexpr auto g = flow::builder<>{}.add(*a >> *b).add(b >> *c); STATIC_REQUIRE(node_size(g) == 3); } -TEST_CASE("edge size (empty flow)", "[graph]") { - constexpr auto g = flow::graph<>{}; +TEST_CASE("edge size (empty flow)", "[builder]") { + constexpr auto g = flow::builder<>{}; STATIC_REQUIRE(edge_size(g) == 1); } -TEST_CASE("edge size (single action)", "[graph]") { - constexpr auto g = flow::graph<>{}.add(*a >> *b); +TEST_CASE("edge size (single action)", "[builder]") { + constexpr auto g = flow::builder<>{}.add(*a >> *b); STATIC_REQUIRE(edge_size(g) == 1); } -TEST_CASE("edge size (overlapping actions)", "[graph]") { - constexpr auto g = flow::graph<>{}.add(*a >> *b).add(*b >> *c); +TEST_CASE("edge size (overlapping actions)", "[builder]") { + constexpr auto g = flow::builder<>{}.add(*a >> *b).add(*b >> *c); STATIC_REQUIRE(edge_size(g) == 2); } diff --git a/test/flow/flow.cpp b/test/flow/flow.cpp index e5b5cdde..4d1302e1 100644 --- a/test/flow/flow.cpp +++ b/test/flow/flow.cpp @@ -49,17 +49,10 @@ TEST_CASE("run empty flow through cib::nexus", "[flow]") { } namespace { -template -using alt_builder = flow::graph; -template > -struct alt_flow_service { - using builder_t = alt_builder; - using interface_t = flow::VizFunctionPtr; - constexpr static auto uninitialized() -> interface_t { return {}; } -}; +template +using alt_builder = flow::builder_for>; -struct VizFlow : public alt_flow_service<"debug"> {}; +struct VizFlow : public flow::service_for> {}; struct VizConfig { constexpr static auto config = cib::config(cib::exports, cib::extend(*a)); diff --git a/test/flow/flow_uninit.cpp b/test/flow/flow_uninit.cpp index 70256b00..7bdc29bf 100644 --- a/test/flow/flow_uninit.cpp +++ b/test/flow/flow_uninit.cpp @@ -1,5 +1,5 @@ #include -#include +#include #include diff --git a/test/flow/graph_builder.cpp b/test/flow/graph_builder.cpp index 56d3d26a..5a5d5f79 100644 --- a/test/flow/graph_builder.cpp +++ b/test/flow/graph_builder.cpp @@ -17,12 +17,12 @@ constexpr auto b = flow::action<"b">([] { actual += "b"; }); constexpr auto c = flow::action<"c">([] { actual += "c"; }); constexpr auto d = flow::action<"d">([] { actual += "d"; }); -using builder = flow::graph_builder<"test_flow", flow::impl>; +using builder = flow::graph_builder<"test_flow">; } // namespace template struct run_flow_t { struct wrapper { - constexpr static auto value = flow::graph<>{}.add(Vs...); + constexpr static auto value = flow::builder<>{}.add(Vs...); }; auto operator()() const -> void { builder::render()(); } @@ -207,8 +207,8 @@ TEST_CASE("add dependency rhs", "[graph_builder]") { } TEST_CASE("reference vs non-reference", "[graph_builder]") { - STATIC_REQUIRE(a.identity == flow::subgraph_identity::REFERENCE); - STATIC_REQUIRE((*a).identity == flow::subgraph_identity::VALUE); + STATIC_REQUIRE(a.identity == flow::dsl::subgraph_identity::REFERENCE); + STATIC_REQUIRE((*a).identity == flow::dsl::subgraph_identity::VALUE); } TEST_CASE("reference in order with non-reference added twice", @@ -219,8 +219,8 @@ TEST_CASE("reference in order with non-reference added twice", #endif TEST_CASE("alternate builder", "[graph_builder]") { - using alt_builder = flow::graphviz_builder; - auto g = flow::graph<"debug">{}.add(*a && (*b >> *c)); + using alt_builder = flow::graphviz_builder<"debug">; + auto g = flow::builder_for{}.add(*a && (*b >> *c)); auto const flow = alt_builder::build(g); auto expected = std::string{ R"__debug__(digraph debug { diff --git a/test/seq/sequencer.cpp b/test/seq/sequencer.cpp index 945e3397..ada3d1b9 100644 --- a/test/seq/sequencer.cpp +++ b/test/seq/sequencer.cpp @@ -10,7 +10,8 @@ namespace { int attempt_count; std::string result{}; -using builder = flow::graph_builder<"test_seq", seq::impl>; +using builder = + flow::graph_builder<"test_seq", flow::log_policies::normal, seq::impl>; } // namespace TEST_CASE("build and run empty seq", "[seq]") { diff --git a/usage_test/main_flow.cpp b/usage_test/main_flow.cpp index 4f613b35..a4cc73bd 100644 --- a/usage_test/main_flow.cpp +++ b/usage_test/main_flow.cpp @@ -1,13 +1,15 @@ #include -#include -#include -#include -#include +#include +#include +#include +#include #include +#include #include #include -#include +#include #include +#include #include #if __STDC_HOSTED__ == 0