diff --git a/include/boost/openmethod/initialize.hpp b/include/boost/openmethod/initialize.hpp index 7f4c4e8c..e4b7dbc7 100644 --- a/include/boost/openmethod/initialize.hpp +++ b/include/boost/openmethod/initialize.hpp @@ -124,27 +124,83 @@ struct initialize_policies> { } }; +// Selects the policies whose state the transaction has to save: those that +// have a `state`, *and* whose `initialize` will actually be called for this +// Context and Options - the same test initialize_policy makes, so the two +// cannot disagree about which policies run. +template +struct policy_state_is_volatile_q { + template + using fn = mp11::mp_bool< + has_policy_state::value && + has_initialize>; +}; + // Saves the policies' states on construction, and puts them back on // destruction unless commit() was called - so a policy's `initialize` that // throws, after itself or another policy has already written to shared // state, leaves the registry as it was. The registry's mutable state is one -// variable, registry_state::st; its `policies` tuple is copied -// whole, states of policies without an `initialize` included, since -// restoring an untouched state is harmless, and simpler than picking. The -// other members need no saving: initialize() only reads the class and -// method lists, and write_global_data() replaces dispatch_data at commit -// time only, after which nothing can throw - on rollback it still holds the -// previous tables, which the classes' static_vptrs point into. (That is -// also why a copy could not stand in for it: it would be another buffer.) -template +// variable, registry_state::st. Its other members need no saving: +// initialize() only reads the class and method lists, and write_global_data() +// replaces dispatch_data at commit time only, after which nothing can throw - +// on rollback it still holds the previous tables, which the classes' +// static_vptrs point into. (That is also why a copy could not stand in for +// it: it would be another buffer.) +// +// Only the states of the policies that are about to be initialized are saved. +// Copying the `policies` tuple whole is simpler, but it is not harmless: a +// state that no `initialize` touches is not derived data this call is about to +// replace, it is configuration the caller owns. The error handler is the case +// that bites - it is *called* from inside the window, by design +// (fast_perfect_hash reports a search failure through it), so a handler that +// disarms itself with set() before throwing would have that undone on the way +// out. Saving wide also forces every policy state in the registry to be +// copyable, including those of policies that have no `initialize` at all - +// which rules out the std::ostringstream an `output` policy written to the +// documented state pattern naturally holds - and copies each of them, vectors +// and all, on every successful initialize(). +template class registry_state_transaction { + using policy_fns = mp11::mp_transform_q< + policy_fn_q, typename Registry::policy_list>; + using saved_states = mp11::mp_transform< + policy_state_t, + mp11::mp_filter_q< + policy_state_is_volatile_q, policy_fns>>; + using saved_type = mp11::mp_apply; + + static_assert( + mp11::mp_all_of::value, + "the `state` of a policy that defines `initialize` must be copyable: " + "initialize() saves it, and puts it back if a policy throws"); + + // Element-wise: `saved` holds a subset of the registry's tuple. + template + struct each; + + template + struct each> { + static void save(detail::tuple& to) { + (..., + (detail::get(to) = + detail::get(Registry::state().policies))); + } + + static void restore(detail::tuple& from) { + (..., + (detail::get(Registry::state().policies) = + std::move(detail::get(from)))); + } + }; + public: - registry_state_transaction() : saved(Registry::state().policies) { + registry_state_transaction() { + each::save(saved); } ~registry_state_transaction() { if (!committed) { - Registry::state().policies = std::move(saved); + each::restore(saved); } } @@ -157,7 +213,7 @@ class registry_state_transaction { } private: - typename registry_state_type::policies_type saved; + saved_type saved; bool committed = false; }; @@ -1878,7 +1934,9 @@ void registry::compiler::write_global_data() { ++tr << rflush(4, dispatch_data_size) << " " << gv_iter << " end\n"; - detail::registry_state_transaction transaction; + detail::registry_state_transaction< + registry, compiler, std::tuple> + transaction; detail::initialize_policies::fn(*this, options); transaction.commit(); @@ -2222,8 +2280,14 @@ void registry::compiler::print_slots() { //! call: no static v-table pointer, `next` pointer, dispatch table or policy //! state is modified. The registry is nonetheless marked as not initialized, //! since that state does not reflect the current registrations; `initialize` -//! must be called again, successfully, before any method is called. Policy -//! states are restored from a copy, so a policy's `state` must be copyable. +//! must be called again, successfully, before any method is called. +//! +//! Only the policies that define `initialize` have their `state` saved and +//! restored, so only those states have to be copyable. A state that no +//! `initialize` writes to is configuration, not derived data, and is left +//! alone: an @ref error_handler policy is *called* during `initialize`, and a +//! handler that changes the configuration - installing a different handler +//! with `set`, say - keeps that change whether the call succeeds or throws. //! //! @par Example //! diff --git a/test/test_initialize_policy_state_scope.cpp b/test/test_initialize_policy_state_scope.cpp new file mode 100644 index 00000000..155bba89 --- /dev/null +++ b/test/test_initialize_policy_state_scope.cpp @@ -0,0 +1,132 @@ +// Copyright (c) 2017-2026 Jean-Louis Leroy +// Distributed under the Boost Software License, Version 1.0. +// See accompanying file LICENSE_1_0.txt +// or copy at http://www.boost.org/LICENSE_1_0.txt) + +// initialize() saves and restores the state of the policies it initializes, +// and only those. A state that no `initialize` writes to is configuration the +// caller owns, not derived data the call is about to replace, so a failed +// initialize() must leave it alone - the error handler is the case that +// matters, since it is called from inside the transaction window by design. +// +// Two consequences, both checked here: such a state survives a rollback, and +// it does not have to be copyable - which it would if the transaction copied +// the whole policy tuple, ruling out the std::ostringstream an `output` policy +// written to the documented state pattern naturally holds. + +#include +#include + +#define BOOST_TEST_MODULE initialize_policy_state_scope +#include + +#include "test_util.hpp" + +#include +#include +#include +#include +#include + +using namespace boost::openmethod; + +// Configuration: it has a `state`, and deliberately no `initialize`. The state +// is move-only, like the std::ostringstream it carries - the transaction must +// not require it to be copyable. +struct config_policy { + using category = config_policy; + + template + struct fn { + struct state { + state() = default; + state(const state&) = delete; + auto operator=(const state&) -> state& = delete; + + int generation = 0; + std::ostringstream os; + }; + }; +}; + +// Writes to its own state and to the configuration policy's - the way an error +// handler called from inside the window changes the handler it installs - then +// throws on demand. +struct explosive_policy { + using category = explosive_policy; + + template + struct fn { + struct state { + int generation = 0; + }; + + inline static bool armed = false; + + template + static void initialize(const Context&, const std::tuple&) { + ++Registry::template state().generation; + ++Registry::template state().generation; + + if (armed) { + throw std::runtime_error("boom"); + } + } + }; +}; + +template +struct test_registry : + test_registry_::template with {}; + +using test_reg = test_registry<__COUNTER__>; +using config_state = config_policy::fn::state; +using explosive = explosive_policy::fn; + +// The point of the exercise: a policy that has no `initialize` may hold a +// state the transaction could not copy even if it wanted to. If this ever +// becomes copyable the test below stops proving anything. +static_assert(!std::is_copy_constructible_v); +static_assert(!std::is_copy_assignable_v); + +struct Animal { + virtual ~Animal() = default; +}; + +struct Dog : Animal {}; + +struct BOOST_OPENMETHOD_ID(poke); + +using poke = method< + BOOST_OPENMETHOD_ID(poke), auto(virtual_)->std::string, test_reg>; + +auto poke_animal(Animal&) -> std::string { + return "silence"; +} + +BOOST_AUTO_TEST_CASE(config_state_survives_a_failed_initialize) { + BOOST_OPENMETHOD_REGISTER(use_classes); + BOOST_OPENMETHOD_REGISTER(poke::override); + + initialize(); + BOOST_TEST(test_reg::state().generation == 1); + BOOST_TEST(test_reg::state().generation == 1); + + explosive::armed = true; + BOOST_CHECK_THROW(initialize(), std::runtime_error); + explosive::armed = false; + + // The initializing policy's own state is derived data: rolled back, so the + // second run's increment is undone. + BOOST_TEST(test_reg::state().generation == 1); + + // The configuration policy's is not: the write made inside the window + // stands, exactly as an error handler that disarms itself before throwing + // would expect. + BOOST_TEST(test_reg::state().generation == 2); + + // And the registry still works. + initialize(); + Dog dog; + BOOST_TEST(poke::fn(dog) == "silence"); +}