From e0f866092d4d3c961ffc3a3aa9dc2f1179ec7e37 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Sun, 7 Feb 2016 01:41:40 +0100 Subject: [PATCH] rectify-design(#301): disentangle CmdClosure hierarchy Completely removed the nested hierarchy, where the top-level implementation forwarded to yet another sub-implementation of the same interface. Rather, this sub-implementation (OpClosure) is now a mere implementation detail class without VTable, and without half-baked re-implementation of the CmdClosure interface. And the state-switch from unbound to bound arguments is now implemented as a plain-flat boolean flag, instead of hiding it in the VTable. To make this possible, without having to rewrite lots of tests, I've created a clone of StorageHolder as a "proof-of-concept" dummy implementation, for the sole purpose of writing test fixtures. This one behaves similar to the real-world thing, but cares only for closing the command operation and omits all the gory details of memento capturing and undo. --- src/lib/meta/function-closure.hpp | 5 +- src/proc/control/command-closure.hpp | 15 +- src/proc/control/command-impl.hpp | 6 +- src/proc/control/command-op-closure.hpp | 61 ++--- src/proc/control/command-simple-closure.hpp | 223 ++++++++++++++++++ src/proc/control/command-storage-holder.hpp | 42 ++-- src/proc/control/memento-tie.hpp | 4 +- tests/45controller.tests | 6 +- .../proc/control/command-equality-test.cpp | 36 +-- .../proc/control/command-mutation-test.cpp | 17 +- 10 files changed, 300 insertions(+), 115 deletions(-) create mode 100644 src/proc/control/command-simple-closure.hpp diff --git a/src/lib/meta/function-closure.hpp b/src/lib/meta/function-closure.hpp index d80a2db9c..9485da941 100644 --- a/src/lib/meta/function-closure.hpp +++ b/src/lib/meta/function-closure.hpp @@ -468,7 +468,10 @@ namespace func{ /** * Closure-creating template. - * @note we take functor objects \em and parameters by reference + * The instance is linked (reference) to a given concrete argument tuple. + * A functor with a matching signature may then either be _closed_ over + * this argument values, or even be invoked right away with theses arguments. + * @warning we take functor objects _and parameters_ by reference */ template class TupleApplicator diff --git a/src/proc/control/command-closure.hpp b/src/proc/control/command-closure.hpp index 8a30eab16..8f231e35a 100644 --- a/src/proc/control/command-closure.hpp +++ b/src/proc/control/command-closure.hpp @@ -101,9 +101,6 @@ namespace control { class CommandImplCloneBuilder; - class CmdClosure; - typedef std::shared_ptr PClo; - /** Interface */ @@ -124,17 +121,7 @@ namespace control { }; - - - - class AbstractClosure - : public CmdClosure - { - bool isValid() const override { return false; } - bool isCaptured() const override { return false; } - void accept (CommandImplCloneBuilder&) const override {} - }; - + using PClo = std::shared_ptr; diff --git a/src/proc/control/command-impl.hpp b/src/proc/control/command-impl.hpp index dea04a8f0..1d00e77f5 100644 --- a/src/proc/control/command-impl.hpp +++ b/src/proc/control/command-impl.hpp @@ -110,14 +110,14 @@ namespace control { * of the embedded FunErasure objects holding the command operation * and undo functors, and the vtable of the embedded CmdClosure */ template - CommandImpl (shared_ptr pArgHolder + CommandImpl (shared_ptr pStorageHolder ,_TY (Func_op) const& operFunctor ,_TY (Func_cap) const& captFunctor ,_TY (Func_undo) const& undoFunctor ) : do_(operFunctor) - , undo_(pArgHolder->tie (undoFunctor, captFunctor)) - , pClo_(pArgHolder) + , undo_(pStorageHolder->tie (undoFunctor, captFunctor)) + , pClo_(pStorageHolder) , defaultPatt_(HandlingPattern::defaultID()) { } diff --git a/src/proc/control/command-op-closure.hpp b/src/proc/control/command-op-closure.hpp index 46fc9cacc..0fd5e42a5 100644 --- a/src/proc/control/command-op-closure.hpp +++ b/src/proc/control/command-op-closure.hpp @@ -126,6 +126,7 @@ namespace control { ParamAccessor& operator= (ParamAccessor const&) =delete; ParamAccessor& operator= (ParamAccessor&&) =delete; ParamAccessor (ParamAccessor const&) =default; + ParamAccessor (ParamAccessor&&) =default; ////////////////////TODO the recursion-end of the access operations goes here @@ -151,7 +152,6 @@ namespace control { */ template class OpClosure - : public AbstractClosure { using Args = typename FunctionSignature< function >::Args; using Builder = BuildTupleAccessor; @@ -159,45 +159,36 @@ namespace control { using ParamStorageTuple =typename Builder::Product; ParamStorageTuple params_; - - protected: - OpClosure() - : params_(Tuple()) - { } + bool activated_; public: using ArgTuple = Tuple; + + OpClosure() + : params_(Tuple()) + , activated_(false) + { } + explicit OpClosure (ArgTuple const& args) : params_(args) + , activated_(true) { } - /** create a clone copy of this, without disclosing the exact type */ - PClo - createClone (TypedAllocationManager& storageManager) + /** we deliberately support immutable types as command arguments */ + OpClosure& operator= (OpClosure const&) =delete; + OpClosure& operator= (OpClosure&&) =delete; + OpClosure (OpClosure const&) =default; + OpClosure (OpClosure&&) =default; + + + bool + isValid () const { - return storageManager.create (*this); + return activated_; } - /** assign a new parameter tuple to this */ - void - bindArguments (Arguments& args) override - { - //params_ = args.get(); - } - - /** assign a new set of parameter values to this. - * @note the values are passed packaged into a sequence - * of GenNode elements. This is the usual way - * arguments are passed from the UI-Bus - */ - void - bindArguments (lib::diff::Rec const& paramData) override - { - //params_ = buildTuple (paramData); - } - /** Core operation: use the embedded argument tuple for invoking a functor * @param unboundFunctor an function object, whose function arguments are @@ -208,7 +199,7 @@ namespace control { * Thus this function can't be const. */ void - invoke (CmdFunctor const& unboundFunctor) override + invoke (CmdFunctor const& unboundFunctor) { TupleApplicator apply_this_arguments(params_); apply_this_arguments (unboundFunctor.getFun()); @@ -216,7 +207,7 @@ namespace control { - operator string() const override + operator string() const { std::ostringstream buff; params_.dump (buff << "OpClosure(" ); @@ -230,19 +221,9 @@ namespace control { } - bool isValid () const override { return true; } - /// Supporting equality comparisons... friend bool operator== (OpClosure const& c1, OpClosure const& c2) { return compare (c1.params_, c2.params_); } friend bool operator!= (OpClosure const& c1, OpClosure const& c2) { return not (c1 == c2); } - - bool - equals (CmdClosure const& other) const override - { - const OpClosure* toCompare = dynamic_cast (&other); - return (toCompare) - && (*this == *toCompare); - } }; diff --git a/src/proc/control/command-simple-closure.hpp b/src/proc/control/command-simple-closure.hpp new file mode 100644 index 000000000..764659d08 --- /dev/null +++ b/src/proc/control/command-simple-closure.hpp @@ -0,0 +1,223 @@ +/* + COMMAND-SIMPLE-CLOSURE.hpp - demo implementation of command closure + + Copyright (C) Lumiera.org + 2016, Hermann Vosseler + + This program is free software; you can redistribute it and/or + modify it under the terms of the GNU General Public License as + published by the Free Software Foundation; either version 2 of + the License, or (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + +*/ + + +/** @file command-simple-closure.hpp + ** Proof-of-concept implementation of CmdClosure. + ** This is used for test only, to invoke an arbitrary matching functor with + ** arguments stored embedded within this closure. In the real system, a more + ** [elaborate version](\ref StorageHolder) of the same concept is used, with + ** the additional complication of managing the UNDO operation as well. + ** + ** \par historical note + ** This proof-of-concept variation was split off in an attempt to improve the + ** overall design of the command / closure system. The original design had the + ** embedded argument holder also implement the CmdClosure interface, which is + ** a clever implementation and code-reuse trick, but otherwise caused confusion. + ** + ** @see Ticket #301 + ** @see CommandMutation_test + ** @see StorageHolder + ** @see OpClosure + ** + */ + + + +#ifndef CONTROL_COMMAND_SIMPLE_CLOSURE_H +#define CONTROL_COMMAND_SIMPLE_CLOSURE_H + +#include "lib/typed-allocation-manager.hpp" +#include "proc/control/command-op-closure.hpp" +#include "lib/opaque-holder.hpp" + +#include + + + +namespace proc { +namespace control { + + using lib::InPlaceBuffer; + using std::string; + + + + + + /** + * Dummy / proof-of-concept implementation of CmdClosure. + * It is a specifically typed subclass, which serves to hold + * storage for the concrete invocation arguments within an + * inline buffer. + * @note for demonstration and unit testing + * @see StorageHolder real world implementation + */ + template + class SimpleClosure + : public CmdClosure + { + using ArgHolder = OpClosure; + using ArgumentBuff = InPlaceBuffer; + + using ArgTuple = typename ArgHolder::ArgTuple; + using Args = typename Types::Seq; + + + /* ====== in-place argument storage ====== */ + + ArgumentBuff arguments_; + + + + /* ==== proxied CmdClosure interface ==== */ + + public: + virtual bool + isValid () const override + { + return arguments_->isValid(); + } + + virtual bool + isCaptured() const override + { + return false; + } + + + + /** assign a new parameter tuple to this */ + virtual void + bindArguments (Arguments& args) override + { + storeTuple (args.get()); + } + + /** assign a new set of parameter values to this. + * @note the values are passed packaged into a sequence + * of GenNode elements. This is the usual way + * arguments are passed from the UI-Bus + */ + virtual void + bindArguments (lib::diff::Rec const& paramData) override + { + storeTuple (buildTuple (paramData)); + } + + + virtual void + invoke (CmdFunctor const& func) override + { + if (!isValid()) + throw lumiera::error::State ("Lifecycle error: can't bind functor, " + "command arguments not yet provided", + LUMIERA_ERROR_UNBOUND_ARGUMENTS); + + arguments_->invoke(func); + } + + + virtual + operator string() const override + { + return "Command-Closure{ arguments=" + + (arguments_->isValid()? string(*arguments_) : "unbound") + + " }" + ; + } + + + + /** per default, all data within StorageHolder + * is set up in \em empty state. Later on, the + * command arguments are to be provided by #bind , + * whereas the undo functions will be wired by #tie + */ + SimpleClosure () + : arguments_() + { } + + explicit + SimpleClosure (ArgTuple const& args) + : arguments_() + { + storeTuple (args); + } + + SimpleClosure (SimpleClosure const& oAh) + : arguments_() + { + if (oAh.arguments_->isValid()) // don't clone garbage from invalid arguments + arguments_.template create (*oAh.arguments_); + } + + + void + accept (CommandImplCloneBuilder&) const override + { + NOTREACHED(); + } + + + /** has undo state capturing been invoked? */ + bool canUndo () const { return false; } + bool empty () const { return !arguments_->isValid(); } + + + /** store a new argument tuple within this StorageHolder, + * discarding any previously stored arguments */ + void + storeTuple (ArgTuple const& argTup) + { + arguments_.template create (argTup); + } + + + bool + equals (CmdClosure const& other) const override + { + const SimpleClosure* toCompare = dynamic_cast (&other); + return (toCompare) + and (*this == *toCompare); + } + + /// Supporting equality comparisons... + friend bool + operator== (SimpleClosure const& a1, SimpleClosure const& a2) + { + return (a1.arguments_->isValid() == a2.arguments_->isValid()) + and (*a1.arguments_ == *a2.arguments_) + ; + } + + friend bool + operator!= (SimpleClosure const& a1, SimpleClosure const& a2) + { + return not (a1 == a2); + } + }; + + + +}} // namespace proc::control +#endif /*CONTROL_COMMAND_SIMPLE_CLOSURE_H*/ diff --git a/src/proc/control/command-storage-holder.hpp b/src/proc/control/command-storage-holder.hpp index 818e0e20e..d8528d0e1 100644 --- a/src/proc/control/command-storage-holder.hpp +++ b/src/proc/control/command-storage-holder.hpp @@ -61,23 +61,6 @@ namespace control { using std::string; - namespace { // empty state marker objects for StorageHolder - - template - struct MissingArguments - : OpClosure - { - virtual bool isValid () const override { return false; } - }; - - - template - struct UntiedMemento - : MementoTie - { }; - - } // (END) impl details / empty state marker objects - @@ -94,17 +77,13 @@ namespace control { */ template class StorageHolder - : public AbstractClosure + : public CmdClosure { - /** copy construction allowed(but no assignment)*/ - StorageHolder& operator= (StorageHolder const&); - - using ArgHolder = OpClosure; using MemHolder = MementoTie; - using ArgumentBuff = InPlaceBuffer>; - using MementoBuff = InPlaceBuffer>; + using ArgumentBuff = InPlaceBuffer; + using MementoBuff = InPlaceBuffer; using ArgTuple = typename ArgHolder::ArgTuple; using Args = typename Types::Seq; @@ -169,7 +148,7 @@ namespace control { operator string() const override { return "Command-State{ arguments=" - + (*arguments_? string(*arguments_) : "unbound") + + (arguments_->isValid()? string(*arguments_) : "unbound") + ", "+string(*memento_)+"}" ; } @@ -186,7 +165,10 @@ namespace control { , memento_() { } - /** copy construction allowed(but no assignment) */ + /** copy construction allowed(but no assignment). + * @remarks rationale is to support immutable argument values, + * which means default/copy construction is OK + */ StorageHolder (StorageHolder const& oAh) : arguments_() , memento_() @@ -198,6 +180,12 @@ namespace control { memento_.template create (*oAh.memento_); } + + /** copy construction allowed(but no assignment)*/ + StorageHolder& operator= (StorageHolder const&) = delete; + + + /** assist with creating a clone copy; * this results in invocation of the copy ctor */ void @@ -259,7 +247,7 @@ namespace control { { const StorageHolder* toCompare = dynamic_cast (&other); return (toCompare) - && (*this == *toCompare); + and (*this == *toCompare); } /// Supporting equality comparisons... diff --git a/src/proc/control/memento-tie.hpp b/src/proc/control/memento-tie.hpp index f5d018966..19ebe5aa5 100644 --- a/src/proc/control/memento-tie.hpp +++ b/src/proc/control/memento-tie.hpp @@ -110,13 +110,11 @@ namespace control { } - protected: + public: MementoTie() : MementoTie (function(), function()) { } - public: - /** creates an execution context tying together the provided functions. * Bound copies of these functors may be pulled from the MementoTie, * in order to build the closures (with the concrete operation arguments) diff --git a/tests/45controller.tests b/tests/45controller.tests index 879294067..398e74db2 100644 --- a/tests/45controller.tests +++ b/tests/45controller.tests @@ -46,9 +46,9 @@ END TEST "Command functor and UNDO functor" CommandMutation_test <>; using ArgHolder = OpClosure; using MemHolder = MementoTie; + using Closure = SimpleClosure; } /*************************************************************************************//** - * @test cover command equality detection. Two commands are deemed equivalent, if they - * - build on the same Mutation functors - * - are either both incomplete or - * - are bound to equivalent arguments - * - hold equivalent undo state (memento) + * @test cover command equality detection. + * Two commands are deemed equivalent, if they + * - build on the same Mutation functors + * - are either both incomplete or + * - are bound to equivalent arguments + * - hold equivalent undo state (memento) * To conduct this test, we set up two sets of functions, and then build both complete * command objects and command implementation facilities based on them. * @@ -171,20 +174,22 @@ namespace test { verifyClosureEquality() { ArgHolder a1 (make_tuple ('a')); - ArgHolder a2 (make_tuple ('z')); + ArgHolder a2 (make_tuple ('a')); + ArgHolder a3 (make_tuple ('z')); CHECK (a1 == a1); - CHECK (a1 != a2); - CHECK (a2 != a1); - - TypedArguments newArgs (make_tuple ('z')); - a1.bindArguments(newArgs); CHECK (a1 == a2); CHECK (a2 == a1); + CHECK (a1 != a3); + CHECK (a3 != a1); + CHECK (a2 != a3); + CHECK (a3 != a2); typedef StorageHolder Storage; Storage abuff1; Storage abuff2; CHECK (abuff1 == abuff2); + + TypedArguments newArgs (make_tuple ('z')); abuff1.bindArguments(newArgs); CHECK (abuff1 != abuff2); abuff2.bindArguments(newArgs); @@ -194,17 +199,18 @@ namespace test { UndoMutation umu2 (abuff2.tie (undo_1, capt_1)); CHECK (abuff1 == abuff2); // same capture function, no memento state! - umu1.captureState(a1); + Closure args {make_tuple ('u')}; + umu1.captureState(args); CHECK (abuff1 != abuff2); - umu2.captureState(a1); + umu2.captureState(args); CHECK (abuff1 == abuff2); // same functions, same memento state check_ += "fake"; // manipulate the "state" to be captured - umu2.captureState(a1); // capture again... + umu2.captureState(args); // capture again... CHECK (abuff1 != abuff2); // captured memento differs! UndoMutation umu3 (abuff2.tie (undo_1, capt_2)); - umu3.captureState(a1); + umu3.captureState(args); CHECK (abuff1 != abuff2); // differing functions detected } diff --git a/tests/core/proc/control/command-mutation-test.cpp b/tests/core/proc/control/command-mutation-test.cpp index 425d1c255..3d1ae2672 100644 --- a/tests/core/proc/control/command-mutation-test.cpp +++ b/tests/core/proc/control/command-mutation-test.cpp @@ -24,7 +24,7 @@ #include "lib/test/run.hpp" #include "lib/test/test-helper.hpp" #include "proc/control/command-mutation.hpp" -#include "proc/control/command-storage-holder.hpp" +#include "proc/control/command-simple-closure.hpp" #include "proc/control/memento-tie.hpp" #include "lib/meta/tuple-helper.hpp" #include "lib/meta/typelist.hpp" @@ -90,8 +90,7 @@ namespace test { } - /** @test check the Mutation functor w#include "lib/meta/typelist.hpp" -hich is bound to our \c testFunc(int) . + /** @test check the Mutation functor which is bound to our `testFunc(int)`. * Then create a argument closure and use this to invoke the Mutation * and verify actually \c testFunc(param) is executed. */ @@ -103,16 +102,16 @@ hich is bound to our \c testFunc(int) . Mutation functor (funky); - MissingArguments nullClosure; - CHECK (!nullClosure); + SimpleClosure nullClosure; + CHECK (not nullClosure.isValid()); cout << "empty placeholder closure: " << nullClosure << endl; VERIFY_ERROR (UNBOUND_ARGUMENTS, functor(nullClosure) ); // now create a real closure.... Tuple > param = std::make_tuple (23); - OpClosure close_over (param); + SimpleClosure closed_over{param}; - CmdClosure& closure (close_over); + CmdClosure& closure (closed_over); CHECK (closure); cout << "param values: " << closure << endl; @@ -152,12 +151,12 @@ hich is bound to our \c testFunc(int) . UndoMutation undoFunctor (mementoHolder); CHECK (!mementoHolder); - MissingArguments nullClosure; + SimpleClosure nullClosure; VERIFY_ERROR (UNBOUND_ARGUMENTS, undoFunctor(nullClosure) ); VERIFY_ERROR (UNBOUND_ARGUMENTS, undoFunctor.captureState(nullClosure) ); Tuple > param; - OpClosure clo (param); + SimpleClosure clo{param}; CHECK (!mementoHolder); VERIFY_ERROR (MISSING_MEMENTO, undoFunctor (clo) );