Inline StringValue into base::Value

base/values.h

Index: base/values.h
diff --git a/base/values.h b/base/values.h
index a695536bfd9a6f8cbb8fd8b3a5e778331e455177..b335e5ca0885d613548223e0a49008bb825405c3 100644
--- a/base/values.h
+++ b/base/values.h
@@ -30,6 +30,7 @@
 #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"
 
@@ -92,6 +93,9 @@ class BASE_EXPORT Value {
   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
@@ -141,15 +145,27 @@ class BASE_EXPORT Value {
   explicit Value(int in_int);
   explicit Value(double in_double);
 
- private:
-  void InternalCopyFrom(const Value& that);
+  // 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);
+  void InternalMoveFrom(Value&& that);
+  void InternalCleanup();
+
   union {
     bool bool_value_;
     int int_value_;
     double double_value_;
+    ManualConstructor<std::string> string_value_;
   };
 };
 
@@ -163,6 +179,7 @@ class BASE_EXPORT FundamentalValue : public 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.
@@ -170,22 +187,6 @@ class BASE_EXPORT StringValue : public 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 {