Convert callers of base::DeepCopy() to base::CreateDeepCopy() in //base

base/json/json_parser.cc

Index: base/json/json_parser.cc
diff --git a/base/json/json_parser.cc b/base/json/json_parser.cc
index 801c84a24cad7d344e6266cd2ccf309b4c976c06..40d358b017901bd873c94f6389ad26429a710318 100644
--- a/base/json/json_parser.cc
+++ b/base/json/json_parser.cc
@@ -5,10 +5,11 @@
 #include "base/json/json_parser.h"
 
 #include <cmath>
-#include <memory>
+#include <utility>
 
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
@@ -34,7 +35,8 @@ const int32_t kExtendedASCIIStart = 0x80;
 // the new instance.
 class DictionaryHiddenRootValue : public DictionaryValue {
  public:
-  DictionaryHiddenRootValue(std::string* json, Value* root) : json_(json) {
+  DictionaryHiddenRootValue(std::string json, Value* root)
+      : json_(std::move(json)) {
     DCHECK(root->IsType(Value::TYPE_DICTIONARY));
     DictionaryValue::Swap(static_cast<DictionaryValue*>(root));
   }
@@ -44,13 +46,13 @@ class DictionaryHiddenRootValue : public DictionaryValue {
 
     // First deep copy to convert JSONStringValue to std::string and swap that
     // copy with |other|, which contains the new contents of |this|.
-    std::unique_ptr<DictionaryValue> copy(DeepCopy());
+    std::unique_ptr<DictionaryValue> copy(CreateDeepCopy());
     copy->Swap(other);
 
     // Then erase the contents of the current dictionary and swap in the
     // new contents, originally from |other|.
     Clear();
-    json_.reset();
+    json_.clear();
     DictionaryValue::Swap(copy.get());
   }
 
@@ -71,20 +73,20 @@ class DictionaryHiddenRootValue : public DictionaryValue {
     if (!DictionaryValue::RemoveWithoutPathExpansion(key, &out_owned))
       return false;
 
-    out->reset(out_owned->DeepCopy());
+    *out = out_owned->CreateDeepCopy();
 
     return true;
   }
 
  private:
-  std::unique_ptr<std::string> json_;
+  std::string json_;
 
   DISALLOW_COPY_AND_ASSIGN(DictionaryHiddenRootValue);
 };
 
 class ListHiddenRootValue : public ListValue {
  public:
-  ListHiddenRootValue(std::string* json, Value* root) : json_(json) {
+  ListHiddenRootValue(std::string json, Value* root) : json_(std::move(json)) {

[dcheng, 2016/04/27 22:12:11]

How do we feel about this? Or do we prefer std::unique_ptr<std::string>? Any
thoughts?

[jbroman, 2016/04/27 22:19:39]

I prefer std::string to std::unique_ptr<std::string>, unless there's some reason
to use unique_ptr which I don't see here.

The string already knows about allocating a buffer, already supports moving,
things are simpler that way, etc.

[jbroman, 2016/04/27 22:25:24]

Having now read more of the surrounding code, I think you _do_ need
std::unique_ptr<std::string> here if you want to be correct. This line:

start_pos_ = input_copy->data();

Takes a pointer into the string buffer, which may become invalid if you move the
std::string. In particular, the "small string" optimization can allocate the
buffer inside the std::string, and so you'd get a different buffer after you
moved (because the short string would be copied).

Otherwise, you'll need to take the const char* after moving the string into the
final Value, not before. That looks like it might be both inconvenient and
subtle.

[dcheng, 2016/04/27 22:38:27]

I don't think start_pos_ is used after parsing. Even in the original code, there
are instances where it'd end up pointing into a destroyed std::string.

     DCHECK(root->IsType(Value::TYPE_LIST));
     ListValue::Swap(static_cast<ListValue*>(root));
   }
@@ -94,13 +96,13 @@ class ListHiddenRootValue : public ListValue {
 
     // First deep copy to convert JSONStringValue to std::string and swap that
     // copy with |other|, which contains the new contents of |this|.
-    std::unique_ptr<ListValue> copy(DeepCopy());
+    std::unique_ptr<ListValue> copy(CreateDeepCopy());
     copy->Swap(other);
 
     // Then erase the contents of the current list and swap in the new contents,
     // originally from |other|.
     Clear();
-    json_.reset();
+    json_.clear();
     ListValue::Swap(copy.get());
   }
 
@@ -117,13 +119,13 @@ class ListHiddenRootValue : public ListValue {
     if (!ListValue::Remove(index, &out_owned))
       return false;
 
-    out->reset(out_owned->DeepCopy());
+    *out = out_owned->CreateDeepCopy();
 
     return true;
   }
 
  private:
-  std::unique_ptr<std::string> json_;
+  std::string json_;
 
   DISALLOW_COPY_AND_ASSIGN(ListHiddenRootValue);
 };
@@ -147,9 +149,11 @@ class JSONStringValue : public Value {
     *out_value = UTF8ToUTF16(string_piece_);
     return true;
   }
+#if 0

[jbroman, 2016/04/27 22:19:39]

remove before landing?

[dcheng, 2016/04/27 22:23:05]

Should already be gone in the latest PS.

   Value* DeepCopy() const override {
     return new StringValue(string_piece_.as_string());
   }
+#endif
   bool Equals(const Value* other) const override {
     std::string other_string;
     return other->IsType(TYPE_STRING) && other->GetAsString(&other_string) &&
@@ -203,14 +207,14 @@ JSONParser::JSONParser(int options)
 JSONParser::~JSONParser() {
 }
 
-Value* JSONParser::Parse(const StringPiece& input) {
-  std::unique_ptr<std::string> input_copy;
+std::unique_ptr<Value> JSONParser::Parse(StringPiece input) {
+  std::string input_copy;
   // If the children of a JSON root can be detached, then hidden roots cannot
   // be used, so do not bother copying the input because StringPiece will not
   // be used anywhere.
   if (!(options_ & JSON_DETACHABLE_CHILDREN)) {
-    input_copy.reset(new std::string(input.as_string()));
-    start_pos_ = input_copy->data();
+    input_copy = input.as_string();
+    start_pos_ = input_copy.data();
   } else {
     start_pos_ = input.data();
   }
@@ -251,19 +255,21 @@ Value* JSONParser::Parse(const StringPiece& input) {
   // hidden root.
   if (!(options_ & JSON_DETACHABLE_CHILDREN)) {
     if (root->IsType(Value::TYPE_DICTIONARY)) {
-      return new DictionaryHiddenRootValue(input_copy.release(), root.get());
+      return WrapUnique(
+          new DictionaryHiddenRootValue(std::move(input_copy), root.get()));
     } else if (root->IsType(Value::TYPE_LIST)) {
-      return new ListHiddenRootValue(input_copy.release(), root.get());
+      return WrapUnique(
+          new ListHiddenRootValue(std::move(input_copy), root.get()));
     } else if (root->IsType(Value::TYPE_STRING)) {
       // A string type could be a JSONStringValue, but because there's no
       // corresponding HiddenRootValue, the memory will be lost. Deep copy to
       // preserve it.
-      return root->DeepCopy();
+      return root->CreateDeepCopy();
     }
   }
 
   // All other values can be returned directly.
-  return root.release();
+  return root;
 }
 
 JSONReader::JsonParseError JSONParser::error_code() const {