gen-node: fix insidious data conssitency problem

I assumed that, since GenNode is composed of copyable and
assignable types, the standard implementation will do.
But I overlooked the run time type check on the opaque
payload type within lib::Variant. When a type mismatch
is detected, the default implementation has already
assigned and thus altered the IDs.

So we need to roll our own implementation, and to add
insult to injury, we can't use the copy-and-swap idiom either.

src/gui/interact/state-map-grouping-storage.hpp

@@ -39,6 +39,7 @@
 #include "lib/error.hpp"
 #include "lib/idi/entry-id.hpp"
 #include "lib/diff/gen-node.hpp"
+#include "lib/util.hpp"
 
 #include <boost/noncopyable.hpp>
 #include <unordered_map>
@@ -52,6 +53,7 @@ namespace interact {
   using lib::idi::BareEntryID;
   using lib::diff::GenNode;
   using lib::diff::Ref;
+  using util::unConst;
   using std::string;
   
   
@@ -117,7 +119,9 @@ namespace interact {
       void
       record (BareEntryID const& elementID, GenNode const& stateMark)
         {
-          elmTable_[elementID].emplace (stateMark);
+          auto res = elmTable_[elementID].emplace (stateMark);
+          if (not res.second)
+            unConst(*res.first) = stateMark; // update existing contents
         }
       
       

src/lib/diff/gen-node.hpp

@@ -269,8 +269,44 @@ namespace diff{
       GenNode(Ref &  r);
       GenNode(Ref && r);
       
-      GenNode& operator= (GenNode const&)  =default;
-      GenNode& operator= (GenNode&&)       =default;
+      /** copy assignment
+       * @remarks we need to define our own version here for sake of sanity.
+       *    The reason is that we use inline storage (embedded within lib::Variant)
+       *    and that we deliberately _erase_ the actual type of data stored inline.
+       *    Because we still do want copy assignment, in case the payload data
+       *    supports this, we use a "virtual copy operator", where in the end
+       *    the storage buffer within lib::Variant has to decide if assignment
+       *    is possible. Only data with the same type may be assigned and we
+       *    prevent change of the (implicit) data type through assignment.
+       *    This check might throw, and for that reason we're better off
+       *    to perform the _data assignment_ first. The probability for
+       *    EntryID assignment to fail is low (but it may happen!).
+       * @note the use of inline storage turns swapping of data
+       *    into an expensive operation, involving a temporary.
+       *    This rules out the copy-and-swap idiom.
+       */
+      GenNode&
+      operator= (GenNode const& o)
+        {
+          if (&o != this)
+            {
+              data = o.data;
+              idi = o.idi;
+            }
+          return *this;
+        }
+      
+      GenNode&
+      operator= (GenNode&& o)
+        {
+          ASSERT (&o != this);
+          data = std::forward<DataCap>(o.data);
+          idi  = std::forward<ID>(o.idi);
+          return *this;
+        }
+      
+      //note: NOT defining a swap operation, because swapping inline storage is pointless!
+      
       
       
       

src/lib/variant.hpp

@@ -452,6 +452,8 @@ namespace lib {
           return *this;
         }
       
+      //note: NOT defining a swap operation, because swapping inline storage is pointless!
+      
       
       /** diagnostic helper */
       operator string()  const;

tests/gui/interact/state-map-grouping-storage-test.cpp

@@ -22,7 +22,7 @@
 
 
 #include "lib/test/run.hpp"
-//#include "lib/test/test-helper.hpp"
+#include "lib/test/test-helper.hpp"
 //#include "gui/ctrl/bus-term.hpp"
 //#include "gui/interact/presentation-state-manager.hpp"
 #include "gui/interact/state-map-grouping-storage.hpp"
@@ -60,7 +60,7 @@ namespace interact{
 namespace test {
   
 //  using proc::control::LUMIERA_ERROR_UNBOUND_ARGUMENTS;
-//  using lumiera::error::LUMIERA_ERROR_WRONG_TYPE;
+  using lumiera::error::LUMIERA_ERROR_WRONG_TYPE;
   
   namespace { // test fixture...
     
@@ -90,9 +90,10 @@ namespace test {
           EntryID<int> quack ("quack");
           
           GenNode poodle{"poodle", "Pudel"};
+          GenNode toyPoodle{"poodle", "Zwergpudel"};
           GenNode pseudoPoodle {"poodle", false};
-          
           GenNode mastiff{"mastiff", "Dogge"};
+          GenNode duck{"duck", "Ente"};
           
           StateMapGroupingStorage storage;
           
@@ -103,10 +104,36 @@ namespace test {
           CHECK (not isnil(storage));
           CHECK (1 == storage.size());
           
-          cout << poodle <<endl;
-          cout << storage.retrieve(woof, "poodle") <<endl;
           CHECK (poodle == storage.retrieve(woof, "poodle"));
           CHECK (not isSameObject (poodle, storage.retrieve(woof, "poodle")));
+          
+          CHECK (Ref::NO == storage.retrieve(wooof, "poodle"));
+          CHECK (Ref::NO == storage.retrieve(woof, "pooodle"));
+          
+          storage.record (woof, mastiff);
+          CHECK (2 == storage.size());
+          CHECK (poodle == storage.retrieve(woof, "poodle"));
+          CHECK (mastiff == storage.retrieve(woof, "mastiff"));
+          
+          // upgrade the poodle
+          storage.record (woof, toyPoodle);
+          CHECK (2 == storage.size());
+          CHECK (poodle    != storage.retrieve(woof, "poodle"));
+          CHECK (toyPoodle == storage.retrieve(woof, "poodle"));
+          
+          // since properties are keyed just by ID-string,
+          // we might attempt sneak in a fake poodle
+          // fortunately GenNode disallows cross-type assignments
+          VERIFY_ERROR (WRONG_TYPE, storage.record (woof, pseudoPoodle) );
+          
+          CHECK (2 == storage.size());
+          cout << toyPoodle <<endl;
+          cout << storage.retrieve(woof, "poodle") <<endl;
+          cout << size_t(toyPoodle.idi.getHash()) <<endl;
+          cout << size_t(storage.retrieve(woof, "poodle").idi.getHash()) <<endl;
+          cout << size_t(pseudoPoodle.idi.getHash()) <<endl;
+          CHECK (toyPoodle == storage.retrieve(woof, "poodle"));
+          CHECK (mastiff == storage.retrieve(woof, "mastiff"));
         }
     };