Merge pull request open-source-parsers#726 from okodron/fix-704

Value::copy() creates a deep copy now

Author: cdunn2001

include/json/value.h

@@ -606,6 +606,9 @@ Json::Value obj_value(Json::objectValue); // {}
 
 private:
   void initBasic(ValueType type, bool allocated = false);
+  void dupPayload(const Value& other);
+  void releasePayload();
+  void dupMeta(const Value& other);
 
   Value& resolveReference(const char* key);
   Value& resolveReference(const char* key, const char* end);

src/lib_json/json_value.cpp

@@ -441,48 +441,9 @@ Value::Value(bool value) {
   value_.bool_ = value;
 }
 
-Value::Value(Value const& other)
-    : type_(other.type_), allocated_(false)
-      ,
-      comments_(0), start_(other.start_), limit_(other.limit_)
-{
-  switch (type_) {
-  case nullValue:
-  case intValue:
-  case uintValue:
-  case realValue:
-  case booleanValue:
-    value_ = other.value_;
-    break;
-  case stringValue:
-    if (other.value_.string_ && other.allocated_) {
-      unsigned len;
-      char const* str;
-      decodePrefixedString(other.allocated_, other.value_.string_,
-          &len, &str);
-      value_.string_ = duplicateAndPrefixStringValue(str, len);
-      allocated_ = true;
-    } else {
-      value_.string_ = other.value_.string_;
-      allocated_ = false;
-    }
-    break;
-  case arrayValue:
-  case objectValue:
-    value_.map_ = new ObjectValues(*other.value_.map_);
-    break;
-  default:
-    JSON_ASSERT_UNREACHABLE;
-  }
-  if (other.comments_) {
-    comments_ = new CommentInfo[numberOfCommentPlacement];
-    for (int comment = 0; comment < numberOfCommentPlacement; ++comment) {
-      const CommentInfo& otherComment = other.comments_[comment];
-      if (otherComment.comment_)
-        comments_[comment].setComment(
-            otherComment.comment_, strlen(otherComment.comment_));
-    }
-  }
+Value::Value(const Value& other) {
+  dupPayload(other);
+  dupMeta(other);
 }
 
 #if JSON_HAS_RVALUE_REFERENCES
@@ -494,24 +455,7 @@ Value::Value(Value&& other) {
 #endif
 
 Value::~Value() {
-  switch (type_) {
-  case nullValue:
-  case intValue:
-  case uintValue:
-  case realValue:
-  case booleanValue:
-    break;
-  case stringValue:
-    if (allocated_)
-      releasePrefixedStringValue(value_.string_);
-    break;
-  case arrayValue:
-  case objectValue:
-    delete value_.map_;
-    break;
-  default:
-    JSON_ASSERT_UNREACHABLE;
-  }
+  releasePayload();
 
   delete[] comments_;
 
@@ -534,9 +478,8 @@ void Value::swapPayload(Value& other) {
 }
 
 void Value::copyPayload(const Value& other) {
-  type_ = other.type_;
-  value_ = other.value_;
-  allocated_ = other.allocated_;
+  releasePayload();
+  dupPayload(other);
 }
 
 void Value::swap(Value& other) {
@@ -548,9 +491,8 @@ void Value::swap(Value& other) {
 
 void Value::copy(const Value& other) {
   copyPayload(other);
-  comments_ = other.comments_;
-  start_ = other.start_;
-  limit_ = other.limit_;
+  delete[] comments_;
+  dupMeta(other);
 }
 
 ValueType Value::type() const { return type_; }
@@ -1049,6 +991,75 @@ void Value::initBasic(ValueType vtype, bool allocated) {
   limit_ = 0;
 }
 
+void Value::dupPayload(const Value& other) {
+  type_ = other.type_;
+  allocated_ = false;
+  switch (type_) {
+  case nullValue:
+  case intValue:
+  case uintValue:
+  case realValue:
+  case booleanValue:
+    value_ = other.value_;
+    break;
+  case stringValue:
+    if (other.value_.string_ && other.allocated_) {
+      unsigned len;
+      char const* str;
+      decodePrefixedString(other.allocated_, other.value_.string_,
+          &len, &str);
+      value_.string_ = duplicateAndPrefixStringValue(str, len);
+      allocated_ = true;
+    } else {
+      value_.string_ = other.value_.string_;
+    }
+    break;
+  case arrayValue:
+  case objectValue:
+    value_.map_ = new ObjectValues(*other.value_.map_);
+    break;
+  default:
+    JSON_ASSERT_UNREACHABLE;
+  }
+}
+
+void Value::releasePayload() {
+  switch (type_) {
+  case nullValue:
+  case intValue:
+  case uintValue:
+  case realValue:
+  case booleanValue:
+    break;
+  case stringValue:
+    if (allocated_)
+      releasePrefixedStringValue(value_.string_);
+    break;
+  case arrayValue:
+  case objectValue:
+    delete value_.map_;
+    break;
+  default:
+    JSON_ASSERT_UNREACHABLE;
+  }
+}
+
+void Value::dupMeta(const Value& other) {
+  if (other.comments_) {
+    comments_ = new CommentInfo[numberOfCommentPlacement];
+    for (int comment = 0; comment < numberOfCommentPlacement; ++comment) {
+      const CommentInfo& otherComment = other.comments_[comment];
+      if (otherComment.comment_)
+        comments_[comment].setComment(
+            otherComment.comment_, strlen(otherComment.comment_));
+    }
+  } else {
+    comments_ = 0;
+  }
+  start_ = other.start_;
+  limit_ = other.limit_;
+}
+
 // Access an object value by name, create a null member if it does not exist.
 // @pre Type of '*this' is object or null.
 // @param key is null-terminated.

src/test_lib_json/main.cpp

@@ -1433,6 +1433,40 @@ JSONTEST_FIXTURE(ValueTest, compareType) {
                                    Json::Value(Json::objectValue)));
 }
 
+JSONTEST_FIXTURE(ValueTest, CopyObject) {
+  Json::Value arrayVal;
+  arrayVal.append("val1");
+  arrayVal.append("val2");
+  arrayVal.append("val3");
+  Json::Value stringVal("string value");
+  Json::Value copy1, copy2;
+  {
+    Json::Value arrayCopy, stringCopy;
+    arrayCopy.copy(arrayVal);
+    stringCopy.copy(stringVal);
+    JSONTEST_ASSERT_PRED(checkIsEqual(arrayCopy, arrayVal));
+    JSONTEST_ASSERT_PRED(checkIsEqual(stringCopy, stringVal));
+    arrayCopy.append("val4");
+    JSONTEST_ASSERT(arrayCopy.size() == 4);
+    arrayVal.append("new4");
+    arrayVal.append("new5");
+    JSONTEST_ASSERT(arrayVal.size() == 5);
+    JSONTEST_ASSERT(!(arrayCopy == arrayVal));
+    stringCopy = "another string";
+    JSONTEST_ASSERT(!(stringCopy == stringVal));
+    copy1.copy(arrayCopy);
+    copy2.copy(stringCopy);
+  }
+  JSONTEST_ASSERT(arrayVal.size() == 5);
+  JSONTEST_ASSERT(stringVal == "string value");
+  JSONTEST_ASSERT(copy1.size() == 4);
+  JSONTEST_ASSERT(copy2 == "another string");
+  copy1.copy(stringVal);
+  JSONTEST_ASSERT(copy1 == "string value");
+  copy2.copy(arrayVal);
+  JSONTEST_ASSERT(copy2.size() == 5);
+}
+
 void ValueTest::checkIsLess(const Json::Value& x, const Json::Value& y) {
   JSONTEST_ASSERT(x < y);
   JSONTEST_ASSERT(y > x);
@@ -2544,6 +2578,7 @@ int main(int argc, const char* argv[]) {
   JSONTEST_REGISTER_FIXTURE(runner, ValueTest, compareArray);
   JSONTEST_REGISTER_FIXTURE(runner, ValueTest, compareObject);
   JSONTEST_REGISTER_FIXTURE(runner, ValueTest, compareType);
+  JSONTEST_REGISTER_FIXTURE(runner, ValueTest, CopyObject);
   JSONTEST_REGISTER_FIXTURE(runner, ValueTest, offsetAccessors);
   JSONTEST_REGISTER_FIXTURE(runner, ValueTest, typeChecksThrowExceptions);
   JSONTEST_REGISTER_FIXTURE(runner, ValueTest, StaticString);