From 77ada853a26bbad461cc40bfb8e1a1de45f68e86 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Fri, 26 Aug 2016 16:29:50 +0200 Subject: [PATCH] verify and clean-up implementation diff application through TreeMutator - esp. verify the proper inclusion of the Selector closure in all Operations - straighten the implementation of Attribute binding - clean-up the error checking helpers --- src/lib/diff/tree-diff-application.hpp | 2 +- src/lib/diff/tree-diff-mutator-binding.cpp | 212 +----------- src/lib/diff/tree-diff-mutator-binding.hpp | 66 +--- .../diff/tree-mutator-attribute-binding.hpp | 65 ++-- .../diff/tree-mutator-collection-binding.hpp | 2 +- wiki/thinkPad.ichthyo.mm | 312 ++++++++++++------ 6 files changed, 260 insertions(+), 399 deletions(-) diff --git a/src/lib/diff/tree-diff-application.hpp b/src/lib/diff/tree-diff-application.hpp index 2e8794471..91182affc 100644 --- a/src/lib/diff/tree-diff-application.hpp +++ b/src/lib/diff/tree-diff-application.hpp @@ -179,7 +179,7 @@ namespace diff{ buildMutator (target); TreeDiffMutatorBinding::scopeManger_ = &scopes_; TreeDiffMutatorBinding::treeMutator_ = &scopes_.currentScope(); - TreeDiffMutatorBinding::initDiffApplication(); + REQUIRE (this->treeMutator_); } }; diff --git a/src/lib/diff/tree-diff-mutator-binding.cpp b/src/lib/diff/tree-diff-mutator-binding.cpp index a92461a20..a2bc6dc4c 100644 --- a/src/lib/diff/tree-diff-mutator-binding.cpp +++ b/src/lib/diff/tree-diff-mutator-binding.cpp @@ -60,189 +60,22 @@ namespace diff{ using std::swap; -#if false /////////////////////////////////////////////////////////////////////////////////////////////////////////////UNIMPLEMENTED :: TICKET #992 - - using Mutator = Rec::Mutator; - using Content = Rec::ContentMutator; - using Iter = Content::Iter; - - template<> - struct TreeDiffMutatorBinding::ScopeFrame - { - Mutator& target; - Content content; - - ScopeFrame(Mutator& toModify) - : target(toModify) - , content() - { } - - void init() - { - target.swapContent (content); - content.resetPos(); - } - }; - - /** Storage: a stack of workspaces - * used to handle nested child objects */ - std::stack scopes_; - - - Mutator& out() { return scopes_.top().target; } - Content& src() { return scopes_.top().content; } - Iter& srcPos() { return scopes_.top().content.pos; } - bool endOfData() { return srcPos() == src().end(); } /////TODO split into an actual scope end check and an non-null check - Rec& alteredRec() { return out(); } - - - void - TreeDiffMutatorBinding::__expect_in_target (GenNode const& elm, Literal oper) - { - if (endOfData()) - throw error::State(_Fmt("Unable to %s element %s from target as demanded; " - "no (further) elements in target sequence") % oper % elm - , LUMIERA_ERROR_DIFF_CONFLICT); - - if (elm.matches(Ref::CHILD) and not srcPos()->isNamed()) - return; // allow for anonymous pick or delete of children - - if (not srcPos()->matches(elm)) - throw error::State(_Fmt("Unable to %s element %s from target as demanded; " - "found element %s on current target position instead") - % oper % elm % *srcPos() - , LUMIERA_ERROR_DIFF_CONFLICT); - } - - void - TreeDiffMutatorBinding::__expect_further_elements (GenNode const& elm) - { - if (endOfData()) - throw error::State(_Fmt("Premature end of target sequence, still expecting element %s; " - "unable to apply diff further.") % elm - , LUMIERA_ERROR_DIFF_CONFLICT); - } - - void - TreeDiffMutatorBinding::__expect_found (GenNode const& elm, Iter const& targetPos) - { - if (targetPos == src().end()) - throw error::State(_Fmt("Premature end of sequence; unable to locate " - "element %s in the remainder of the target.") % elm - , LUMIERA_ERROR_DIFF_CONFLICT); - } - - void - TreeDiffMutatorBinding::__expect_successful_location (GenNode const& elm) - { - if (endOfData() - and not ( elm.matches(Ref::END) // after(_END_) -> its OK we hit the end - or (elm.matches(Ref::ATTRIBS) and src().children.empty()))) // after(_ATTRIBS_) -> if there are no children, it's OK to hit the end - throw error::State(_Fmt("Unable locate position 'after(%s)'") % elm.idi - , LUMIERA_ERROR_DIFF_CONFLICT); - } - - void - TreeDiffMutatorBinding::__expect_valid_parent_scope (GenNode::ID const& idi) - { - if (scopes_.empty()) - throw error::State(_Fmt("Unbalanced child scope bracketing tokens in diff; " - "When leaving scope %s, we fell out of root scope.") % idi.getSym() - , LUMIERA_ERROR_DIFF_CONFLICT); - - if (alteredRec().empty()) - throw error::State(_Fmt("Corrupted state. When leaving scope %s, " - "we found an empty parent scope.") % idi.getSym() - , LUMIERA_ERROR_DIFF_CONFLICT); - } - - void - TreeDiffMutatorBinding::__expect_end_of_scope (GenNode::ID const& idi) - { - if (not endOfData()) - throw error::State(_Fmt("Incomplete diff: when about to leave scope %s, " - "not all previously existing elements have been confirmed by the diff. " - "At least one spurious element %s was left over") % idi.getSym() % *srcPos() - , LUMIERA_ERROR_DIFF_CONFLICT); - } - - - Iter - TreeDiffMutatorBinding::find_in_current_scope (GenNode const& elm) - { - Iter end_of_scope = src().currIsAttrib()? src().attribs.end() - : src().children.end(); - return std::find_if (srcPos() - ,end_of_scope - ,[&](auto& entry) - { - return entry.matches(elm); - }); - } - - GenNode const& - TreeDiffMutatorBinding::find_child (GenNode::ID const& idi) - { - if (alteredRec().empty()) - throw error::State(_Fmt("Attempt to mutate element %s, but current target data scope is empty. " - "Sender and receiver out of sync?") % idi.getSym() - , LUMIERA_ERROR_DIFF_CONFLICT); - - // Short-cut-mutation: look at the last element. - // this should be the one just added. BUT NOTE: this fails - // when adding an attribute after entering the child scope. - // Since attributes are typically values and not mutated, - // this inaccuracy was deemed acceptable - auto& current = out().accessLast(); - if (Ref::THIS.matches(idi) or current.matches(idi)) - return current; - - for (auto & child : alteredRec()) - if (child.idi == idi) - return child; - - throw error::State(_Fmt("Attempt to mutate non existing child record; unable to locate child %s " - "after applying the diff. Current scope: %s") % idi.getSym() % alteredRec() - , LUMIERA_ERROR_DIFF_CONFLICT); - } - - void - TreeDiffMutatorBinding::move_into_new_sequence (Iter pos) - { - if (src().currIsAttrib()) - out().appendAttrib (move(*pos)); //////////////TICKET #969 was it a good idea to allow adding attributes "after the fact"? - else - out().appendChild (move(*pos)); - } -#endif /////////////////////////////////////////////////////////////////////////////////////////////////////////////UNIMPLEMENTED :: TICKET #992 /* == Forwarding: error handling == */ void - TreeDiffMutatorBinding::__expect_in_target (GenNode const& elm, Literal oper) + TreeDiffMutatorBinding::__failMismatch (Literal oper, GenNode const& spec) { - - if (endOfData()) - throw error::State(_Fmt("Unable to %s element %s from target as demanded; " - "no (further) elements in target sequence") % oper % elm - , LUMIERA_ERROR_DIFF_CONFLICT); - -// /////////////////////////////////////////////////////////////TODO: is there any chance to uphold this special syntactic construct?? otherwise drop it from the language! -// if (elm.matches(Ref::CHILD) and not srcPos()->isNamed()) -// return; // allow for anonymous pick or delete of children - - if (not treeMutator_->matchSrc(elm)) - throw error::State(_Fmt("Unable to %s element %s from target as demanded; " - "found element %s on current target position instead") - % oper % elm % srcPos() /////////////////////TODO still relevant? - , LUMIERA_ERROR_DIFF_CONFLICT); + throw error::State(_Fmt("Unable to %s element %s. Current shape of target " + "data does not match expectations") % oper % spec + , LUMIERA_ERROR_DIFF_CONFLICT); } void TreeDiffMutatorBinding::__expect_further_elements (GenNode const& elm) { - if (endOfData()) + if (not treeMutator_->hasSrc()) throw error::State(_Fmt("Premature end of target sequence, still expecting element %s; " "unable to apply diff further.") % elm , LUMIERA_ERROR_DIFF_CONFLICT); @@ -256,14 +89,6 @@ namespace diff{ , LUMIERA_ERROR_DIFF_CONFLICT); } - void - TreeDiffMutatorBinding::__failMismatch (GenNode const& spec, Literal oper) - { - throw error::State(_Fmt("Unable to %s element %s. Current shape of target " - "data does not match expectations") % oper % spec - , LUMIERA_ERROR_DIFF_CONFLICT); - } - void TreeDiffMutatorBinding::__expect_end_of_scope (GenNode::ID const& idi) { @@ -293,23 +118,25 @@ namespace diff{ { bool success = treeMutator_->injectNew(n); if (not success) - __failMismatch (n, "insert"); + __failMismatch ("insert", n); } void TreeDiffMutatorBinding::del (GenNode const& n) { - __expect_in_target(n, "remove"); + __expect_further_elements(n); + if (not treeMutator_->matchSrc(n)) + __failMismatch("remove", n); + treeMutator_->skipSrc(n); } void TreeDiffMutatorBinding::pick (GenNode const& n) { -// __expect_in_target(n, "pick"); ////////////////////////TODO TOD-o ? bool success = treeMutator_->acceptSrc (n); if (not success) - __failMismatch (n, "pick"); + __failMismatch ("pick", n); } void @@ -329,6 +156,8 @@ namespace diff{ } + + /* == Implementation of the tree diff application primitives == */ /** cue to a position behind the named node, @@ -346,7 +175,7 @@ namespace diff{ TreeDiffMutatorBinding::set (GenNode const& n) { if (not treeMutator_->assignElm(n)) - __failMismatch (n, "assign"); + __failMismatch("assign", n); } /** open nested scope to apply diff to child object */ @@ -355,7 +184,7 @@ namespace diff{ { TreeMutator::Handle buffHandle = scopeManger_->openScope(); if (not treeMutator_->mutateChild(n, buffHandle)) - __failMismatch (n, "enter nested scope"); + __failMismatch("enter nested scope", n); TRACE (diff, "tree-diff: ENTER scope %s", cStr(n.idi)); treeMutator_ = buffHandle.get(); @@ -374,15 +203,4 @@ namespace diff{ - void - TreeDiffMutatorBinding::initDiffApplication() - { - REQUIRE (scopeManger_); - REQUIRE (treeMutator_); - } - - - - - }} // namespace lib::diff diff --git a/src/lib/diff/tree-diff-mutator-binding.hpp b/src/lib/diff/tree-diff-mutator-binding.hpp index e07c5cc67..429e12536 100644 --- a/src/lib/diff/tree-diff-mutator-binding.hpp +++ b/src/lib/diff/tree-diff-mutator-binding.hpp @@ -216,74 +216,12 @@ namespace diff{ ScopeManager* scopeManger_; private: -#if false /////////////////////////////////////////////////////////////////////////////////////////////////////////////UNIMPLEMENTED :: TICKET #992 - using Mutator = Rec::Mutator; - using Content = Rec::ContentMutator; - using Iter = Content::Iter; - - struct ScopeFrame - { - Mutator& target; - Content content; - - ScopeFrame(Mutator& toModify) - : target(toModify) - , content() - { } - - void init() - { - target.swapContent (content); - content.resetPos(); - } - }; - - /** Storage: a stack of workspaces - * used to handle nested child objects */ - std::stack scopes_; - - - Mutator& out() { return scopes_.top().target; } - Content& src() { return scopes_.top().content; } - Iter& srcPos() { return scopes_.top().content.pos; } - bool endOfData() { return srcPos() == src().end(); } /////TODO split into an actual scope end check and an non-null check - Rec& alteredRec() { return out(); } - - - void __expect_in_target (GenNode const& elm, Literal oper); - void __expect_further_elements (GenNode const& elm); - void __expect_found (GenNode const& elm, Iter const& targetPos); - void __expect_successful_location (GenNode const& elm); - void __expect_valid_parent_scope (GenNode::ID const& idi); - void __expect_end_of_scope (GenNode::ID const& idi); - - - Iter find_in_current_scope (GenNode const& elm); - - GenNode const& find_child (GenNode::ID const& idi); - - void move_into_new_sequence (Iter pos); -#endif /////////////////////////////////////////////////////////////////////////////////////////////////////////////UNIMPLEMENTED :: TICKET #992 - - bool - endOfData() - { - return not treeMutator_->hasSrc(); - } - - string - srcPos() - { - return "??"; ////////////////TODO can this be served by the treeMutator_ ?? Is it still necessary? - } - /* == Forwarding: error handling == */ - void __expect_in_target (GenNode const& elm, Literal oper); + void __failMismatch (Literal oper, GenNode const& spec); void __expect_further_elements (GenNode const& elm); void __fail_not_found (GenNode const& elm); - void __failMismatch (GenNode const& spec, Literal oper); void __expect_end_of_scope (GenNode::ID const& idi); void __expect_valid_parent_scope (GenNode::ID const& idi); @@ -312,8 +250,6 @@ namespace diff{ : treeMutator_(nullptr) , scopeManger_(nullptr) { } - - void initDiffApplication(); }; diff --git a/src/lib/diff/tree-mutator-attribute-binding.hpp b/src/lib/diff/tree-mutator-attribute-binding.hpp index 04efbf141..1167e6dfb 100644 --- a/src/lib/diff/tree-mutator-attribute-binding.hpp +++ b/src/lib/diff/tree-mutator-attribute-binding.hpp @@ -105,6 +105,15 @@ //== anonymous namespace... + /** + * Generic behaviour of any binding to object fields (attributes). + * Since object fields as such are part of the class definition, a diff + * will never be able to add, insert, delted or re-order fields. Thus we + * do not need to keep track of an "old" and "new" order; rather there + * is always one single fixed element present to work on. + * @note consequently, several diff operations are either implemented NOP, + * or passed to the parent (lower onion layers). + */ template class AttributeBindingBase : public PAR @@ -132,22 +141,20 @@ and attribID_ == spec.idi; } + void + __ifApplicable_refuse_to(Literal oper, GenNode const& spec) + { + if (this->isApplicable(spec)) + throw error::Logic (_Fmt{"attempt to %s attribute '%s', " + "but this binding for '%s' is linked to a data field and " + "thus does not support any notion of 'order' or 'position', " + "inserting or deletion."} + % oper % spec.idi % this->attribID_); + } + /* ==== re-Implementation of the operation API ==== */ public: - /** this binding to an object field can not support any reordering, - * inserting or deletion of "Elements", since the structure of an object - * is fixed through the underlying class definition. For this reason, - * we do not keep track of an "old" and "new" order; rather there - * is always one single fixed element present to work on. - * @return `true` always - */ - virtual bool - hasSrc() override - { - return true; // x or true == true - } - /** ensure the given spec is deemed appropriate at that point. * Due to the hard wired nature of an object field binding, this can * only be verified passively: a spec targeted at an unknown attribute @@ -160,6 +167,10 @@ or PAR::matchSrc (spec); } + + // note: hasSrc() not overridden here --> delegate to parent layer + + /** accept status quo, after verifying the spec from the diff verb */ virtual bool acceptSrc (GenNode const& spec) override @@ -177,25 +188,15 @@ virtual void skipSrc (GenNode const& refSpec) override { - if (isApplicable(refSpec)) - throw error::Logic (_Fmt{"attempt to skip or drop attribute '%s', " - "which is bound to an object field and " - "thus can not cease to exist."} - % refSpec.idi.getSym()); - else - PAR::skipSrc (refSpec); + __ifApplicable_refuse_to ("skip or drop", refSpec); + PAR::skipSrc (refSpec); } virtual bool findSrc (GenNode const& refSpec) override { - if (isApplicable(refSpec)) - throw error::Logic (_Fmt{"attempt to re-order attribute '%s', " - "which is bound to an object field and " - "thus does not respond to ordering."} - % refSpec.idi.getSym()); - else - return PAR::findSrc (refSpec); + __ifApplicable_refuse_to ("re-order", refSpec); + return PAR::findSrc (refSpec); } /** there is no real support for navigating to a 'position', @@ -216,14 +217,10 @@ if (Ref::ATTRIBS == spec) return true; else - if (isApplicable(spec)) - throw error::Logic (_Fmt{"attempt to navigate to a position behind attribute '%s', " - "but this binding for '%s' is linked to a data field and " - "thus does not support any notion of 'order' or 'position'."} - % spec.idi.getSym() - % this->attribID_); - else + { + __ifApplicable_refuse_to ("navigate to a position behind", spec); return PAR::accept_until(spec); + } } }; diff --git a/src/lib/diff/tree-mutator-collection-binding.hpp b/src/lib/diff/tree-mutator-collection-binding.hpp index d1d207073..934a8eb3d 100644 --- a/src/lib/diff/tree-mutator-collection-binding.hpp +++ b/src/lib/diff/tree-mutator-collection-binding.hpp @@ -220,7 +220,7 @@ virtual bool hasSrc () override { - return pos_; + return bool(pos_) or PAR::hasSrc(); } /** ensure the next recorded source element diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index e63d71440..9d659c67d 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -35,7 +35,7 @@ - + @@ -45,9 +45,10 @@ heißt: Element registriert sich am UI-Bus

- + + - + @@ -57,11 +58,12 @@ heißt: Element deregistriert sich am UI-Bus

- + + - + @@ -71,7 +73,8 @@ ...ist immer ein tangible

- + + @@ -121,7 +124,7 @@ - + @@ -131,10 +134,11 @@ dafür genügt der normale Reset

- + + - + @@ -144,9 +148,10 @@ mark "clearMsg"

- + + - + @@ -156,9 +161,10 @@ mark "clearErr"

- + + - + @@ -168,7 +174,8 @@ mark "reset"

- + + @@ -193,7 +200,7 @@ - + @@ -216,9 +223,10 @@ was haben alle UI-Elemente wirklich gemeinsam?

- + + - + @@ -234,7 +242,8 @@ oder handelt es sich nur um ein Implementierungsdetail der UI-Bus-Anbindung?

- + + @@ -309,7 +318,7 @@ - + @@ -322,10 +331,11 @@ Dann mußte das allerdigns jeweils für alle Elemente sinnvoll sein

- + + - + @@ -335,7 +345,8 @@ und der muß vom konkreten Widget implementiert werden

- + + @@ -475,7 +486,7 @@ - + @@ -497,10 +508,11 @@ Und ich muß das in einem Test zumindest emulieren können!

- + + - + @@ -537,7 +549,8 @@ - + + @@ -564,7 +577,7 @@ - + @@ -590,7 +603,8 @@ ist jedoch schon prototypisch implementiert

- + + @@ -814,7 +828,7 @@ - + @@ -835,13 +849,14 @@ - + + - + @@ -862,7 +877,8 @@ - + + @@ -1004,7 +1020,7 @@ - + @@ -1014,7 +1030,8 @@ since skipSrc performs both the `del` and the `skip` verb, it can not perform the match itself...

- + + @@ -1022,7 +1039,7 @@ - + @@ -1053,7 +1070,8 @@ to the next lower layer in both cases, and the result and behaviour depends on this next lower layer solely

- + + @@ -1081,7 +1099,7 @@ - + @@ -1103,10 +1121,11 @@ of this specific onion layer to accept forward until meeting this element.

- + + - + @@ -1132,7 +1151,8 @@ will check the bool return value and throw an exception in that case

- + + @@ -1197,7 +1217,7 @@ - + @@ -1213,7 +1233,8 @@ Wichtigster solcher Fall ist die Bindung auf Objekt-Felder

- + + @@ -1243,7 +1264,7 @@ - + @@ -1262,7 +1283,8 @@ to construct the new target data efficiently in place.

- + + @@ -1852,7 +1874,7 @@ - + @@ -1865,7 +1887,8 @@ durch den das Problem mit der "absrakten, opaquen" Position entschärft wird

- + + @@ -2057,7 +2080,7 @@ - + @@ -2074,7 +2097,7 @@ - + @@ -2093,7 +2116,8 @@ und protokolliert somit "nebenbei" was an Anforderungen an ihm vorbeigeht

- + + @@ -2308,7 +2332,7 @@ - + @@ -2330,7 +2354,7 @@ - + @@ -2343,8 +2367,9 @@ für spezifische Arten von Bindings

- - + + + @@ -2353,7 +2378,7 @@ - + @@ -2385,12 +2410,13 @@ denn sonst würde er es für darunter liegende Layer verschatten.

- + + - + @@ -2817,10 +2843,10 @@ - + - + @@ -2839,7 +2865,8 @@ dann müssen Attribute irgendwie sinnvoll integriert sein

- + + @@ -4282,9 +4309,9 @@ - - - + + + @@ -4307,7 +4334,7 @@ - + @@ -4329,10 +4356,11 @@ In jedem Fall gerät dadurch die relative Verzahnung der Elemente untereinander aus dem Takt

- + + - + @@ -4357,7 +4385,8 @@ und diese Elemente müssen geschlossen hintereinander in der Reihenfolge liegen

- + + @@ -4380,7 +4409,7 @@ - + @@ -4401,11 +4430,32 @@ - + + - + + + + + + +

+ ...das so häufig in C++ auftretende Problem: +

+

+ wie baue und verwalte ich eine konkrete Implementierung, +

+

+ ohne gleich ein ganzes Management-Framework einführen zu müssen. +

+

+ Letzten Endes lief  das auch in diesem Fall auf inline-Storage hinaus... +

+ + + @@ -4458,7 +4508,7 @@ - + @@ -4486,7 +4536,8 @@ Standard == Interface DiffMutable implementieren

- + + @@ -4749,12 +4800,12 @@ - + - - - - + + + + @@ -4766,7 +4817,7 @@ - + @@ -4777,9 +4828,9 @@

- - - + + + @@ -4797,20 +4848,38 @@ - + + - + - + - - - + + + - + + + + + + +

+ ...das heißt: +

+

+ gegeben ein syntaktisch sinnvoller top-level-Aufruf ("wende das Diff an") +

+

+ -- wie bzw. von wem bekommen wir dann ein Binding, das einen passenden TreeMutator konstruiert? +

+ + + - + @@ -4820,6 +4889,9 @@ + + + @@ -4828,7 +4900,7 @@ - + @@ -4932,7 +5004,8 @@ - + + @@ -4998,14 +5071,16 @@ - + + + - + @@ -5015,10 +5090,11 @@ ...denn es ist sehr verwirrend, welche Signatur denn nun die Lambdas haben müssen

- + + - + @@ -5028,7 +5104,8 @@ ...denn es kann keinen Default-Matcher geben....

- + + @@ -5038,7 +5115,7 @@ - + @@ -5054,7 +5131,8 @@ daß der Client hier eigentlich ein Protokoll implementieren muß.

- + + @@ -5070,7 +5148,8 @@ - + + @@ -5082,7 +5161,37 @@ - + + + + + + + + + + + + + + +

+ wenn überhaupt, dann im Matcher im Binding-Layer implementieren +

+ + + + + + + + + + + + + + @@ -5519,7 +5628,7 @@ - + @@ -5532,7 +5641,8 @@ Implementierung der real-world-Variante fehlt!

- + +