Assignment with move semantics violates POLA

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Create a document jsonDoc ;
2. Assign a value in the document to a new rapidjson::GenericValue< 
rapidjson::UTF8<> > newVariable = jsonDoc[ "SomeValue" ]
3. jsonDoc is now null, by design

What is the expected output? What do you see instead?
jsonDoc should refer to the root of the tree.

Original issue reported on code.google.com by billy.ba...@gmail.com on 9 Feb 2012 at 3:05

[GoogleCodeExporter]

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?

Original comment by stephe...@gmail.com on 9 Feb 2012 at 3:53

[GoogleCodeExporter]

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.

Original comment by billy.ba...@gmail.com on 9 Feb 2012 at 7:09

[GoogleCodeExporter]

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. 

Original comment by milo...@gmail.com on 19 Feb 2012 at 4:00

[GoogleCodeExporter]

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?

Original comment by billy.ba...@gmail.com on 19 Feb 2012 at 4:38

[GoogleCodeExporter]

Need to revise this API. Planned for 0.2 version.

Original comment by milo...@gmail.com on 14 Nov 2012 at 3:45

• Changed state: Accepted
• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

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?

Original comment by Duane.Mu...@gmail.com on 8 Aug 2013 at 8:57

[GoogleCodeExporter]

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

Original comment by philipp....@gmail.com on 7 Apr 2014 at 12:14

[GoogleCodeExporter]

Original comment by milo...@gmail.com on 24 Jun 2014 at 2:15

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

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!

Original comment by philipp....@gmail.com on 24 Jun 2014 at 2:21

[GoogleCodeExporter]

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

Original comment by milo...@gmail.com on 26 Jun 2014 at 2:39

• Changed state: Fixed