Assignment with move semantics violates POLA about rapidjson HOT 10 CLOSED

So assigning a node/branch of a tree nullifies the whole tree? That's going to 
surprise a lot of people. Maybe there should be an option to disallow 
assignment with move?

So let's assume you have a json document object A, and you assign B = A, where 
B is another json document object.


Well, in the source at line 160 of document.h, 

        //! Assignment with move semantics.
    /*! \param rhs Source of the assignment. It will become a null value after assignment.
    */
    GenericValue& operator=(GenericValue& rhs) {
        RAPIDJSON_ASSERT(this != &rhs);
        this->~GenericValue();
        memcpy(this, &rhs, sizeof(GenericValue));
        rhs.flags_ = kNullFlag; //!! makes A null, in assignment B = A
        return *this;
    }

So, in my source, I commented out the line

        rhs.flags_ = kNullFlag;

And that actually works _for now_.  It doesn't seem to have caused any 
problems, but I can't be sure if there are negative side effects.

And yes, if you have a tree like:

FILE: objects.json:

{
  "fruits":{
    "oranges":4,
    "bananas":2,
    "apples":3
  },
  "vegs":{
    "beans":8,
    "peas":20
  }
}

And you do something like:

  rapidjson::Document jsonDoc = load( "objects.json" ) ; // assume it loads the file

Then you later do:

  rapidjson::Value fruits = jsonDoc[ "fruits" ] ;


  // jsonDoc now NULL, SURPRISE!
  rapidjson::Value vegs = jsonDoc[ "veg" ] ; // ERROR

Problem is, api doesn't seem to expose _any_ easy way to walk the document tree 
when there are nested objects.

When document/value are traversed, it should be probably use references instead 
of new objects, as in tutorial.cpp:

 const Value& a = document["a"];    // Using a reference for consecutive access is handy and faster.

For your change:
 rhs.flags_ = kNullFlag;

This will not work appropriately if rhs is an array or an object. This makes 
two values shares the same array/object. And it will even crash if the 
allocator is not MemoryPoolAllocator (the array/object will be destroy twice).

The rationale behind is that cloning nodes can be expensive (a deep clone of a 
DOM tree/sub-tree) and this operation should not be common in 
traversing/modifying a json DOM, so that the behavior of assignment uses move 
semantics by design.

I think more examples can be added to help user understand this behavior at the 
mean time.

In future, a Clone() function can be added to let user explicitly cloning a DOM 
tree/sub-tree.

Thank you for your comment. 

So I think it's a hole in the API.  You shouldn't allow programmers to shoot 
themselves in the foot so easily.

Perhaps return a pointer from operator[] instead?

Need to revise this API. Planned for 0.2 version.

It seems like supporting swap would be a good way of dealing with not 
supporting assignment. Prior to C++11, the moral equivalent of assignment with 
move (WAT!) is swap.

With that in mind, can swap be implemented outside of document?

For the record: There is an implementation proposal for adding an explicit 
"copy constructor" and a "CopyFrom" function in my personal fork a Github [1].

As an answer to #6: Yes, a swap function can already be implemented easily 
outside of GenericValue/GenericDocument:

  template<typename Encoding, typename Allocator>
  void swap(GenericValue<Encoding,Allocator> & v1, GenericValue<Encoding,Allocator> & v2) {
    GenericValue<Encoding,Allocator> tmp;
    tmp = v2;
    v2 = v1;
    v1 = tmp;
  }

Description of the pull-request:

To allow deep copying from an existing GenericValue, an explicit "copy 
constructor" (with required Allocator param) and a CopyFrom assignment function 
are added.

  Document d; Document::AllocatorType& a = d.GetAllocator();
  Value v1("foo");
  // Value v2(v1); // not allowed

  Value v2(v1,a);                             // make a copy
  RAPIDJSON_ASSERT(v1.IsString());            // v1 untouched
  d.SetArray().PushBack(v1,a).PushBack(v2,a);
  RAPIDJSON_ASSERT(v1.Empty() && v2.Empty());

  v2.CopyFrom(d,a);                           // copy whole document
  RAPIDJSON_ASSERT(d.IsArray() && d.Size());  // d untouched
  v1.SetObject().AddMember( "array", v2, a );
  d.PushBack(v1,a);

The strings in the source value are copied, if and only if they have been 
allocated as a copy during their construction (determined by kCopyFlag). This 
is needed to avoid double free()s or problems with differing lifetimes of the 
allocated string storage.

Additionally, the Handler implementation in GenericDocument is made private 
again, restricting access to GenericReader and GenericValue.

Changes: https://github.com/pah/rapidjson/compare/svn/trunk...value-copy-from

[1]: GenericValue: add copy constructor and CopyFrom
     https://github.com/pah/rapidjson/pull/2

Milo, please give me some time to open official pull-requests at GitHub before 
adding the patches pasted here (and to the other issues).  It would be nice to 
keep the correct author attribution in the revision history.  Thanks!

https://github.com/miloyip/rapidjson/pull/20