From 0782dd492213cdf015d7ef0c4942a07299120f37 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Tue, 9 Aug 2016 23:42:42 +0200 Subject: [PATCH] investigate and confirm the logic underlying the matchSrc, skipSrc and acceptSrc primitives In Theory, acceptSrc and skipSrc are to operate symmetrically, with the sole difference that skipSrc does not move anything into the new content. BUT, since skipSrc is also used to implement the `skip` verb, which serves to discard garbage left back by a preceeding `find`, we cannot touch the data found in the src position without risk of SEGFAULT. For this reason, there is a dedicated matchSrc operation, which shall be used to generate the verification step to properly implement the `del` verb. I've spent quite some time to verify the logic of predicate evaluation. It seems to be OK: whenever the SELECTOR applies, then we'll perform the local match, and then also we'll perform the skipSrc. Otherwise, we'll delegate both operations likewise to the next lower layer, without touching anything here. --- src/lib/diff/test-mutation-target.hpp | 38 +++++---- src/lib/diff/tree-diff-mutator-binding.cpp | 8 +- src/lib/diff/tree-diff-mutator-binding.hpp | 1 - .../diff/tree-mutator-attribute-binding.hpp | 12 +++ .../diff/tree-mutator-collection-binding.hpp | 36 ++++----- src/lib/diff/tree-mutator.hpp | 24 ++++-- .../diff/tree-mutator-binding-test.cpp | 8 +- wiki/thinkPad.ichthyo.mm | 78 +++++++++++++++---- 8 files changed, 135 insertions(+), 70 deletions(-) diff --git a/src/lib/diff/test-mutation-target.hpp b/src/lib/diff/test-mutation-target.hpp index 808f06a8b..909a4b80b 100644 --- a/src/lib/diff/test-mutation-target.hpp +++ b/src/lib/diff/test-mutation-target.hpp @@ -376,26 +376,12 @@ namespace diff{ /* ==== re-Implementation of the operation API ==== */ - /** skip next recorded src element - * @remarks TestWireTap adapter together with TestMutationTarget - * maintain a "shadow copy" of the data and apply the detected diff - * against this internal copy. This allows to verify what's going on - */ - virtual void - skipSrc (GenNode const& n) override - { - if (pos_) - { - GenNode const& skippedElm = *pos_; - ++pos_; - target_.logSkip (skippedElm); - } - PAR::skipSrc(n); - } - /** record in the test target * that a new child element is * being inserted at current position + * @remarks TestWireTap adapter together with TestMutationTarget + * maintain a "shadow copy" of the data and apply the detected diff + * against this internal copy. This allows to verify what's going on */ virtual bool injectNew (GenNode const& n) override @@ -417,8 +403,20 @@ namespace diff{ matchSrc (GenNode const& n) override { return PAR::matchSrc(n) - or pos_? n.matches(*pos_) - : false; + or pos_ and n.matches(*pos_); + } + + /** skip next recorded src element without touching it */ + virtual void + skipSrc (GenNode const& n) override + { + if (pos_) + { + GenNode const& skippedElm = *pos_; + ++pos_; + target_.logSkip (skippedElm); + } + PAR::skipSrc(n); } /** accept existing element, when matching the given spec */ @@ -426,7 +424,7 @@ namespace diff{ acceptSrc (GenNode const& n) override { bool isSrcMatch = pos_ and n.matches(*pos_); - if (isSrcMatch) // NOTE: important to invoke our own match here + if (isSrcMatch) //NOTE: important to invoke our own match here { target_.inject (move(*pos_), "acceptSrc"); ++pos_; diff --git a/src/lib/diff/tree-diff-mutator-binding.cpp b/src/lib/diff/tree-diff-mutator-binding.cpp index 283d3ea22..c4869a992 100644 --- a/src/lib/diff/tree-diff-mutator-binding.cpp +++ b/src/lib/diff/tree-diff-mutator-binding.cpp @@ -265,12 +265,6 @@ namespace diff{ UNIMPLEMENTED("skip next src element and advance abstract source position"); } - bool - TreeDiffMutatorBinding::injectNew (GenNode const& n) - { - return treeMutator_->injectNew(n); - } - bool TreeDiffMutatorBinding::matchSrc (GenNode const& n) { @@ -320,7 +314,7 @@ namespace diff{ void TreeDiffMutatorBinding::ins (GenNode const& n) { - bool success = injectNew (n); + bool success = treeMutator_->injectNew(n); if (not success) __failMismatch (n, "insert"); } diff --git a/src/lib/diff/tree-diff-mutator-binding.hpp b/src/lib/diff/tree-diff-mutator-binding.hpp index b50cad24b..fd567149c 100644 --- a/src/lib/diff/tree-diff-mutator-binding.hpp +++ b/src/lib/diff/tree-diff-mutator-binding.hpp @@ -279,7 +279,6 @@ namespace diff{ /* == Forwarding: mutation primitives == */ void skipSrc(); - bool injectNew (GenNode const& n); bool matchSrc (GenNode const& n); bool acceptSrc (GenNode const& n); bool findSrc (GenNode const& n); diff --git a/src/lib/diff/tree-mutator-attribute-binding.hpp b/src/lib/diff/tree-mutator-attribute-binding.hpp index e40098511..6a9a226c7 100644 --- a/src/lib/diff/tree-mutator-attribute-binding.hpp +++ b/src/lib/diff/tree-mutator-attribute-binding.hpp @@ -174,6 +174,18 @@ * in a way not to ask this binding to "reorder" a field from an * existing class definition. */ + 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); + } + virtual bool findSrc (GenNode const& refSpec) override { diff --git a/src/lib/diff/tree-mutator-collection-binding.hpp b/src/lib/diff/tree-mutator-collection-binding.hpp index 60870a18b..c969f3a6a 100644 --- a/src/lib/diff/tree-mutator-collection-binding.hpp +++ b/src/lib/diff/tree-mutator-collection-binding.hpp @@ -200,21 +200,6 @@ /* ==== re-Implementation of the operation API ==== */ - /** skip next pending src element, - * causing this element to be discarded - */ - virtual void - skipSrc (GenNode const& n) override - { - if (binding_.isApplicable(n)) - { - if (pos_) - ++pos_; - } - else - PAR::skipSrc (n); - } - /** fabricate a new element, based on * the given specification (GenNode), * and insert it at current position @@ -244,12 +229,27 @@ matchSrc (GenNode const& spec) override { if (binding_.isApplicable(spec)) - return pos_? binding_.matches (spec, *pos_) - : false; + return pos_ and binding_.matches (spec, *pos_); else return PAR::matchSrc (spec); } + /** skip next pending src element, + * causing this element to be discarded + * @note can not perform a match on garbage data + */ + virtual void + skipSrc (GenNode const& n) override + { + if (binding_.isApplicable(n)) + { + if (pos_) + ++pos_; + } + else + PAR::skipSrc (n); + } + /** accept existing element, when matching the given spec */ virtual bool acceptSrc (GenNode const& n) override @@ -257,7 +257,7 @@ if (binding_.isApplicable(n)) { bool isSrcMatch = pos_ and binding_.matches (n, *pos_); - if (isSrcMatch) // NOTE: crucial to perform only our own match check here + if (isSrcMatch) //NOTE: crucial to perform only our own match check here { binding_.inject (move(*pos_)); ++pos_; diff --git a/src/lib/diff/tree-mutator.hpp b/src/lib/diff/tree-mutator.hpp index 54858ea85..2803c2417 100644 --- a/src/lib/diff/tree-mutator.hpp +++ b/src/lib/diff/tree-mutator.hpp @@ -210,14 +210,6 @@ namespace diff{ // do nothing by default } - /** skip next src element and advance abstract source position. - * The argument shall be used to determine applicability */ - virtual void - skipSrc (GenNode const&) - { - // do nothing by default - } - /** establish new element at current position * @return `true` when successfully inserted something */ virtual bool @@ -235,6 +227,22 @@ namespace diff{ return false; } + /** skip next src element and advance abstract source position. + * The argument shall be used to determine applicability + * @remarks this operation is used both to implement the `del` verb + * and the `skip` verb. Since the latter discards garbage + * left back by `find` we must not touch the contents, + * to prevent a SEGFAULT. Thus `skipSrc` can not match + * and thus can not return anything. Consequently the + * `del` implementation has to use `matchSrc` explicitly, + * and the latter must invoke the selector prior to + * performing the local match. */ + virtual void + skipSrc (GenNode const&) + { + // do nothing by default + } + /** accept existing element, when matching the given spec */ virtual bool acceptSrc (GenNode const&) diff --git a/tests/library/diff/tree-mutator-binding-test.cpp b/tests/library/diff/tree-mutator-binding-test.cpp index 4f54a6a36..dfdbc7b9e 100644 --- a/tests/library/diff/tree-mutator-binding-test.cpp +++ b/tests/library/diff/tree-mutator-binding-test.cpp @@ -766,7 +766,7 @@ namespace test{ gamma = val; }); - CHECK (sizeof(mutator1) <= sizeof(void*) // the TreeMutator VTable + CHECK (sizeof(mutator2) <= sizeof(void*) // the TreeMutator VTable + 3 * sizeof(void*) // one closure reference for each lambda + 3 * sizeof(GenNode::ID)); // one attribute-key for each binding @@ -789,7 +789,8 @@ namespace test{ CHECK (mutator2.matchSrc (ATTRIB1)); // behaviour of the binding remains unaffected CHECK (mutator2.acceptSrc (ATTRIB1)); // now pick and "accept" this src element (also a NOP) // acceptSrc - mutator2.skipSrc (ATTRIB1); // and 'skip' likewise is just not implemented for attributes // skipSrc + VERIFY_ERROR (LOGIC, mutator2.skipSrc (ATTRIB3)); + // and 'skip' likewise is just not implemented for attributes // skipSrc CHECK ( 1 == alpha); CHECK (-1 == beta); CHECK (3.45 == gamma); // all these non-operations actually didn't change anything... @@ -807,7 +808,8 @@ namespace test{ // but since all those operations are not relevant for our attribute binding, they will be passed on // to lower binding layers. And since, moreover, there /are no lower binding layers/ in our setup, // they will just do nothing and return false - mutator2.skipSrc (ATTRIB3); // skipSrc + CHECK (not mutator2.matchSrc (CHILD_B)); + mutator2.skipSrc (CHILD_B); // ...no setter binding, thus no effect // skipSrc CHECK (not mutator2.injectNew (SUB_NODE));// ...no setter binding, thus no effect // injectNew CHECK (not mutator2.matchSrc (CHILD_B)); CHECK (not mutator2.acceptSrc (CHILD_B)); // acceptSrc diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 3e11ada62..dbb99426e 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -973,6 +973,11 @@ + + + + + @@ -994,38 +999,85 @@ - - - - - - - + + + + + + +

+ since skipSrc performs both the `del` and the `skip` verb, it can not perform the match itself... +

+ + + - + + - - + + + + + + + + + +

+ ...because it is also used to discard garbage after a findSrc operation. +

+

+ Thus we need to avoid touching the actual data in the src sequence, because this might lead to SEGFAULT. +

+

+ +

+

+ For this reason, the implementation of the `del` verb has to invoke matchSrc explicitly beforehand, +

+

+ and this is the very reason `matchSrc` exists. Moreover, `matchSrc` must be written such +

+

+ as to ensure to invoke the Selector before performing a local match. And skipSrc has to +

+

+ proceed in precisely the same way. Thus, if the selector denies responsibility, we'll delegate +

+

+ to the next lower layer in both cases, and the result and behaviour depends on this next lower layer solely +

+ + + + + - + + + + +

- move into target + then move into target

- + +