Inline StringValue into base::Value

base/values.h

 // 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.
 
 // This file specifies a recursive data storage class called Value intended for
 // storing settings and other persistable data.
 //
 // A Value represents something that can be stored in JSON or passed to/from
 // JavaScript. As such, it is NOT a generalized variant type, since only the
 // types supported by JavaScript/JSON are supported.
@@ skipping 12 matching lines @@
 #include <iosfwd>
 #include <map>
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/base_export.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/manual_constructor.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_piece.h"
 
 namespace base {
 
 class BinaryValue;
 class DictionaryValue;
 class FundamentalValue;
 class ListValue;
 class StringValue;
@@ skipping 42 matching lines @@
   bool is_double() const { return type() == Type::DOUBLE; }
   bool is_string() const { return type() == Type::STRING; }
   bool is_blob() const { return type() == Type::BINARY; }
   bool is_dict() const { return type() == Type::DICTIONARY; }
   bool is_list() const { return type() == Type::LIST; }
 
   // These will all fatally assert if the type doesn't match.
   bool GetBool() const;
   int GetInt() const;
   double GetDouble() const;  // Implicitly converts from int if necessary.
+  // Returns |string_value_| as a non-const or const reference.
+  std::string& GetString();

[jdoerrie, 2016/12/16 09:54:37]

For now I added the non-const ref version for simplicity. vabr@ and I discussed
offline how SetString should work, and we decided that we probably need 4
versions:

- SetString(const char*)
- SetString(const std::string&)
- SetString(std::string&&)
- SetString(const base::string16&)

Do you agree with this, brettw@? A couple of questions remain, however:
- Do we plan on supporting SetString for Values of type() != Type::STRING?
- If we add SetString, should we also add SetInt, SetDouble, etc?

[vabr (Chromium), 2016/12/16 17:00:56]

Just for context: part of the discussion was also about replacing the middle two
with just SetString(std::string) with std::move inside. For cases when a copy is
being made, the latter always does heap allocation, whereas SetString(const
base::string&) skips the allocation if the currently held string is long enough.
To choose the right approach, we would need to know what is the typical use and
perhaps measure that.
I left a comment in the design doc about this [1]. In short, to keep the API
simple, we should not offer multiple ways to do things. And resetting the
StringValue is already possible by creating a new one and assigning it to the
current one. We should just ensure that that operation is efficient.

[1]
https://docs.google.com/document/d/1uDLu5uTRlCWePxQUEHc8yNQdEoE1BDISYdpggWEAB...

+  const std::string& GetString() const;
 
   // These methods allow the convenient retrieval of the contents of the Value.
   // If the current object can be converted into the given type, the value is
   // returned through the |out_value| parameter and true is returned;
   // otherwise, false is returned and |out_value| is unchanged.
   virtual bool GetAsBoolean(bool* out_value) const;
   virtual bool GetAsInteger(int* out_value) const;
   virtual bool GetAsDouble(double* out_value) const;
   virtual bool GetAsString(std::string* out_value) const;
   virtual bool GetAsString(string16* out_value) const;
@@ skipping 29 matching lines @@
   Value(const Value& that);
   Value(Value&& that);
   Value& operator=(const Value& that);
   Value& operator=(Value&& that);
 
   // TODO(brettw) move to public when FundamentalValue is gone.
   explicit Value(bool in_bool);
   explicit Value(int in_int);
   explicit Value(double in_double);
 
+  // TODO(crbug.com/646113) move to public when StringValue is gone.
+  explicit Value(StringPiece in_string);

[jdoerrie, 2016/12/16 09:54:37]

Given that we need an explicit Value::Value(const char*), this should only be
called for std::string arguments. Should we replace this in favor of
Value::Value(const std::string&) and Value::Value(std::string&&)?

[vabr (Chromium), 2016/12/16 17:00:56]

Definitely in favour of replacing by string-based constructors (see my comment
above). In addition, not relying on implicit conversions will make the behaviour
of the API (esp. wrt. copies) clearer.

+  explicit Value(const string16& in_string);
+
+  // Required despite StringPiece because otherwise the compiler will
+  // choose the Value(bool) constructor.
+  explicit Value(const char* in_string);

[vabr (Chromium), 2016/12/16 17:00:55]

As a TODO to be done in a separate CL, could we transform the callsites to pass
StringPieces and mark this constructor as =delete (or private) to avoid the
fall-back to Value(bool)? The goal being that the API would stay smaller and
there would only be one way to pass a pointer to a C-string -- by upgrading it
to a StringPiece.

[jdoerrie, 2016/12/19 11:54:09]

If it is your goal to stop being able to do |Value val("foobar")|, we actually
don't need to declare a const char* constructor at all, the implicit conversion
to bool already raises compilation errors:

error: implicit conversion turns string literal into bool: 'const char [7]' to
'bool' [-Werror,-Wstring-conversion]

However, making it private or marking it deleted definitely makes our intent
clearer.

Just out of curiosity, what is the value of only allowing StringPieces? Both
StringPieces and const char*s would need to be copied to the internal
std::string, correct?

[vabr (Chromium), 2016/12/19 13:00:56]

Good to know about the error. However, because it is not self-evident what is
the benefit of forbidding char*-based constructor, we might need to document it
well to keep it that way. The explicit =delete with some explanation in a
comment would be good for that.
That's correct. My slight concern with char* is that it is too easy to be
surprised with tricks like converting it to a bool. char* is a C-language way to
work with strings, whereas StringPiece is a C++ way of being explicit of what
that data is and what actions on it are expected/allowed.

+
+  // TODO(crbug.com/646113): Make this private once JSONStringValue is removed.
+  Type type_;
+
  private:
   void InternalCopyFrom(const Value& that);
-
-  Type type_;
+  void InternalMoveFrom(Value&& that);
+  void InternalCleanup();
 
   union {
     bool bool_value_;
     int int_value_;
     double double_value_;
+    ManualConstructor<std::string> string_value_;
   };
 };
 
 // FundamentalValue represents the simple fundamental types of values.
 // TODO(brettw) remove when callers are updated to use raw Value.
 class BASE_EXPORT FundamentalValue : public Value {
  public:
   explicit FundamentalValue(bool in_value);
   explicit FundamentalValue(int in_value);
   explicit FundamentalValue(double in_value);
   ~FundamentalValue() override;
 };
 
+// TODO(crbug.com/64113) remove when callers are updated to use raw Value.
 class BASE_EXPORT StringValue : public Value {

[vabr (Chromium), 2016/12/16 17:00:56]

Could we actually make this a typedef instead? (For that we would need to make
the string-related Value() constructors public:

using StringValue = Value;

Doing so would make it clearer that StringValue is no more different, and also
would avoid explicit down-casts via static_cast.

[jdoerrie, 2016/12/19 11:54:09]

This is true, but it probably also would require making the copy and move
constructors and assignment operators public. Currently there's a comment that
making the constructors public would not be safe, so I am not quite sure what to
do here.

[vabr (Chromium), 2016/12/19 13:00:56]

I'm not sure the comment about safety applies to all constructors. IMO that's
just about the ones allowing to set the type and nothing else. Perhaps Brett can
confirm or refute this?

  public:
   // Initializes a StringValue with a UTF-8 narrow character string.
   explicit StringValue(StringPiece in_value);
 
   // Initializes a StringValue with a string16.
   explicit StringValue(const string16& in_value);
-
-  ~StringValue() override;
-
-  // Returns |value_| as a pointer or reference.
-  std::string* GetString();
-  const std::string& GetString() const;
-
-  // Overridden from Value:
-  bool GetAsString(std::string* out_value) const override;
-  bool GetAsString(string16* out_value) const override;
-  bool GetAsString(const StringValue** out_value) const override;
-  StringValue* DeepCopy() const override;
-  bool Equals(const Value* other) const override;
-
- private:
-  std::string value_;
 };
 
 class BASE_EXPORT BinaryValue: public Value {
  public:
   // Creates a BinaryValue with a null buffer and size of 0.
   BinaryValue();
 
   // Creates a BinaryValue, taking ownership of the bytes pointed to by
   // |buffer|.
   BinaryValue(std::unique_ptr<char[]> buffer, size_t size);
@@ skipping 378 matching lines @@
   return out << static_cast<const Value&>(value);
 }
 
 // Stream operator so that enum class Types can be used in log statements.
 BASE_EXPORT std::ostream& operator<<(std::ostream& out,
                                      const Value::Type& type);
 
 }  // namespace base
 
 #endif  // BASE_VALUES_H_