Cleaning up base::Value assignment.

base/values.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/values.h"
 
 #include <string.h>
 
 #include <algorithm>
 #include <cmath>
@@ skipping 155 matching lines @@
 
 Value::Value(const std::vector<char>& in_blob) : type_(Type::BINARY) {
   binary_value_.Init(in_blob);
 }
 
 Value::Value(std::vector<char>&& in_blob) : type_(Type::BINARY) {
   binary_value_.Init(std::move(in_blob));
 }
 
 Value& Value::operator=(const Value& that) {
-  if (this != &that) {
-    if (type_ == that.type_) {
-      InternalCopyAssignFromSameType(that);
-    } else {
-      InternalCleanup();
-      InternalCopyConstructFrom(that);
-    }
+  if (type_ == that.type_) {
+    InternalCopyAssignFromSameType(that);
+  } else {
+    // this is not a self assignment because the type_ doesn't match.

[vabr (Chromium), 2017/03/13 10:56:01]

this -> This

[dyaroshev, 2017/03/13 21:15:42]

Done.

+    InternalCleanup();
+    InternalCopyConstructFrom(that);
   }
 
   return *this;
 }
 
 Value& Value::operator=(Value&& that) {
-  if (this != &that) {
-    if (type_ == that.type_) {
-      InternalMoveAssignFromSameType(std::move(that));
-    } else {
-      InternalCleanup();
-      InternalMoveConstructFrom(std::move(that));
-    }
-  }
+  DCHECK(this != &that) << " attempt to self move assign.";

[dcheng, 2017/03/11 06:30:07]

Super minor nit: no space at the beginning of the string

[dyaroshev, 2017/03/13 21:15:42]

Done.

+  InternalCleanup();
+  InternalMoveConstructFrom(std::move(that));
 
   return *this;
 }
 
 Value::~Value() {
   InternalCleanup();
 }
 
 // static
 const char* Value::GetTypeName(Value::Type type) {
@@ skipping 319 matching lines @@
     case Type::DICTIONARY:
       dict_ptr_.InitFromMove(std::move(that.dict_ptr_));
       return;
     case Type::LIST:
       list_.InitFromMove(std::move(that.list_));
       return;
   }
 }
 
 void Value::InternalCopyAssignFromSameType(const Value& that) {
   CHECK_EQ(type_, that.type_);

[dcheng, 2017/03/11 06:30:07]

While you're fixing this, do mind changing these to DCHECKs? I don't see a
reason why they should be CHECKs.

[vabr (Chromium), 2017/03/13 10:56:01]

The reason is potential execution of random pieces of memory. Please do not
remove the checks. Please see https://codereview.chromium.org/2728773005/ for
more context.

[dyaroshev, 2017/03/13 21:15:42]

I see - you worried about slicing causing execution of random code.
Would you think that it's ok to remove these CHECKs after value conversion is
done?
It has a measurable performance impact (not very big though).

[dcheng, 2017/03/14 05:08:01]

While it's true that we should be using CHECK when there are security
implications... this is an internal function that's called in one place, by one
thing. An internal class helper should be allowed to assume that its own
implementation will satisfy the preconditions, and use DCHECK to enforce it.

 
   switch (type_) {
     case Type::NONE:
     case Type::BOOLEAN:
     case Type::INTEGER:
     case Type::DOUBLE:
       InternalCopyFundamentalValue(that);
       return;
 
     case Type::STRING:
       *string_value_ = *that.string_value_;
       return;
     case Type::BINARY:
       *binary_value_ = *that.binary_value_;
       return;
     // DictStorage and ListStorage are move-only types due to the presence of
     // unique_ptrs. This is why the call to |CreateDeepCopy| is necessary here.
     // TODO(crbug.com/646113): Clean this up when DictStorage and ListStorage
     // can be copied directly.
     case Type::DICTIONARY:
       *dict_ptr_ = std::move(*that.CreateDeepCopy()->dict_ptr_);
       return;
     case Type::LIST:
       *list_ = std::move(*that.CreateDeepCopy()->list_);
       return;
   }
 }
 
-void Value::InternalMoveAssignFromSameType(Value&& that) {
-  CHECK_EQ(type_, that.type_);
-
-  switch (type_) {
-    case Type::NONE:
-    case Type::BOOLEAN:
-    case Type::INTEGER:
-    case Type::DOUBLE:
-      InternalCopyFundamentalValue(that);
-      return;
-
-    case Type::STRING:
-      *string_value_ = std::move(*that.string_value_);
-      return;
-    case Type::BINARY:
-      *binary_value_ = std::move(*that.binary_value_);
-      return;
-    case Type::DICTIONARY:
-      *dict_ptr_ = std::move(*that.dict_ptr_);
-      return;
-    case Type::LIST:
-      *list_ = std::move(*that.list_);
-      return;
-  }
-}
-
 void Value::InternalCleanup() {
   switch (type_) {
     case Type::NONE:
     case Type::BOOLEAN:
     case Type::INTEGER:
     case Type::DOUBLE:
       // Nothing to do
       return;
 
     case Type::STRING:
@@ skipping 754 matching lines @@
 }
 
 std::ostream& operator<<(std::ostream& out, const Value::Type& type) {
   if (static_cast<int>(type) < 0 ||
       static_cast<size_t>(type) >= arraysize(kTypeNames))
     return out << "Invalid Type (index = " << static_cast<int>(type) << ")";
   return out << Value::GetTypeName(type);
 }
 
 }  // namespace base