Overload on dynamic object reference type.

Summary: There are a bunch of methods in `folly::dynamic` that return a
reference to a sub-object, e.g. `getString()`.
These methods are a bit unsafe because it allows you to write code with dangling
references when the `folly::dynamic` object is an rvalue:

  auto& obj = parse_json(some_string).at("key");

However, the bigger win is that when we have an rvalue `folly::dynamic`, such as
returned by `getDefault(...)`, we can move the sub-object out:

  auto obj = parse_json(some_string).at("key");
  auto str = obj.getDefault("another-key", "").getString();

In the first line, the subobject is copied out when it could be safely moved out.
In the second line `getDefault(...)` copies the dynamic sub-object out, and then
`getString()` copies the string out, when the string could be moved.

Also in the case of `getDefault()` being called on a rvalue, we can move the
sub-object out.

Reviewed By: @marcinpe

Differential Revision: D2267588

diff --git a/folly/dynamic-inl.h b/folly/dynamic-inl.h
index 6b42fab2b93d99e7c54162b451e122be84659050..6e2fd62a290705d5d0e7f7902e2512a9e1bf8005 100644 (file)
--- a/folly/dynamic-inl.h
+++ b/folly/dynamic-inl.h
@@ -384,15 +384,20 @@ inline double   dynamic::asDouble() const { return asImpl<double>(); }
 inline int64_t  dynamic::asInt()    const { return asImpl<int64_t>(); }
 inline bool     dynamic::asBool()   const { return asImpl<bool>(); }
 
-inline const fbstring& dynamic::getString() const { return get<fbstring>(); }
-inline double          dynamic::getDouble() const { return get<double>(); }
-inline int64_t         dynamic::getInt()    const { return get<int64_t>(); }
-inline bool            dynamic::getBool()   const { return get<bool>(); }
+inline const fbstring& dynamic::getString() const& { return get<fbstring>(); }
+inline double          dynamic::getDouble() const& { return get<double>(); }
+inline int64_t         dynamic::getInt()    const& { return get<int64_t>(); }
+inline bool            dynamic::getBool()   const& { return get<bool>(); }
 
-inline fbstring& dynamic::getString() { return get<fbstring>(); }
-inline double&   dynamic::getDouble() { return get<double>(); }
-inline int64_t&  dynamic::getInt()    { return get<int64_t>(); }
-inline bool&     dynamic::getBool()   { return get<bool>(); }
+inline fbstring& dynamic::getString() & { return get<fbstring>(); }
+inline double&   dynamic::getDouble() & { return get<double>(); }
+inline int64_t&  dynamic::getInt()    & { return get<int64_t>(); }
+inline bool&     dynamic::getBool()   & { return get<bool>(); }
+
+inline fbstring dynamic::getString() && { return std::move(get<fbstring>()); }
+inline double   dynamic::getDouble() && { return get<double>(); }
+inline int64_t  dynamic::getInt()    && { return get<int64_t>(); }
+inline bool     dynamic::getBool()   && { return get<bool>(); }
 
 inline const char* dynamic::data()  const& { return get<fbstring>().data();  }
 inline const char* dynamic::c_str() const& { return get<fbstring>().c_str(); }
@@ -460,10 +465,14 @@ inline dynamic& dynamic::operator--() {
   return *this;
 }
 
-inline dynamic const& dynamic::operator[](dynamic const& idx) const {
+inline dynamic const& dynamic::operator[](dynamic const& idx) const& {
   return at(idx);
 }
 
+inline dynamic dynamic::operator[](dynamic const& idx) && {
+  return std::move((*this)[idx]);
+}
+
 template<class K, class V> inline dynamic& dynamic::setDefault(K&& k, V&& v) {
   auto& obj = get<ObjectImpl>();
   return obj.insert(std::make_pair(std::forward<K>(k),
@@ -474,10 +483,14 @@ inline dynamic* dynamic::get_ptr(dynamic const& idx) & {
   return const_cast<dynamic*>(const_cast<dynamic const*>(this)->get_ptr(idx));
 }
 
-inline dynamic& dynamic::at(dynamic const& idx) {
+inline dynamic& dynamic::at(dynamic const& idx) & {
   return const_cast<dynamic&>(const_cast<dynamic const*>(this)->at(idx));
 }
 
+inline dynamic dynamic::at(dynamic const& idx) && {
+  return std::move(at(idx));
+}
+
 inline bool dynamic::empty() const {
   if (isNull()) {
     return true;

diff --git a/folly/dynamic.cpp b/folly/dynamic.cpp
index f7be57e8ca0dc5e9cb5fc00a014bee0aecb74313..d743bb2296831a541f756b714cfaf475522ee965 100644 (file)
--- a/folly/dynamic.cpp
+++ b/folly/dynamic.cpp
@@ -132,7 +132,7 @@ dynamic& dynamic::operator=(dynamic&& o) noexcept {
   return *this;
 }
 
-dynamic& dynamic::operator[](dynamic const& k) {
+dynamic& dynamic::operator[](dynamic const& k) & {
   if (!isObject() && !isArray()) {
     throw TypeError("object/array", type());
   }
@@ -144,20 +144,38 @@ dynamic& dynamic::operator[](dynamic const& k) {
   return ret.first->second;
 }
 
-dynamic dynamic::getDefault(const dynamic& k, const dynamic& v) const {
+dynamic dynamic::getDefault(const dynamic& k, const dynamic& v) const& {
   auto& obj = get<ObjectImpl>();
   auto it = obj.find(k);
   return it == obj.end() ? v : it->second;
 }
 
-dynamic&& dynamic::getDefault(const dynamic& k, dynamic&& v) const {
+dynamic dynamic::getDefault(const dynamic& k, dynamic&& v) const& {
   auto& obj = get<ObjectImpl>();
   auto it = obj.find(k);
-  if (it != obj.end()) {
-    v = it->second;
+  // Avoid clang bug with ternary
+  if (it == obj.end()) {
+    return std::move(v);
+  } else {
+    return it->second;
   }
+}
 
-  return std::move(v);
+dynamic dynamic::getDefault(const dynamic& k, const dynamic& v) && {
+  auto& obj = get<ObjectImpl>();
+  auto it = obj.find(k);
+  // Avoid clang bug with ternary
+  if (it == obj.end()) {
+    return v;
+  } else {
+    return std::move(it->second);
+  }
+}
+
+dynamic dynamic::getDefault(const dynamic& k, dynamic&& v) && {
+  auto& obj = get<ObjectImpl>();
+  auto it = obj.find(k);
+  return std::move(it == obj.end() ? v : it->second);
 }
 
 const dynamic* dynamic::get_ptr(dynamic const& idx) const& {
@@ -180,7 +198,7 @@ const dynamic* dynamic::get_ptr(dynamic const& idx) const& {
   }
 }
 
-dynamic const& dynamic::at(dynamic const& idx) const {
+dynamic const& dynamic::at(dynamic const& idx) const& {
   if (auto* parray = get_nothrow<Array>()) {
     if (!idx.isInt()) {
       throw TypeError("int64", idx.type());

diff --git a/folly/dynamic.h b/folly/dynamic.h
index 35ef9b5ccb2390e06431b1961c59a7ff834a786b..bea7fc442e9fa9f1182eb5219fc1640904b2e0ed 100644 (file)
--- a/folly/dynamic.h
+++ b/folly/dynamic.h
@@ -283,14 +283,18 @@ public:
    *
    * These will throw a TypeError if the dynamic has a different type.
    */
-  const fbstring& getString() const;
-  double          getDouble() const;
-  int64_t         getInt() const;
-  bool            getBool() const;
-  fbstring& getString();
-  double&   getDouble();
-  int64_t&  getInt();
-  bool&     getBool();
+  const fbstring& getString() const&;
+  double          getDouble() const&;
+  int64_t         getInt() const&;
+  bool            getBool() const&;
+  fbstring& getString() &;
+  double&   getDouble() &;
+  int64_t&  getInt() &;
+  bool&     getBool() &;
+  fbstring getString() &&;
+  double   getDouble() &&;
+  int64_t  getInt() &&;
+  bool     getBool() &&;
 
   /*
    * It is occasionally useful to access a string's internal pointer
@@ -362,8 +366,9 @@ public:
    * will throw a TypeError.  Using an index that is out of range or
    * object-element that's not present throws std::out_of_range.
    */
-  dynamic const& at(dynamic const&) const;
-  dynamic&       at(dynamic const&);
+  dynamic const& at(dynamic const&) const&;
+  dynamic&       at(dynamic const&) &;
+  dynamic        at(dynamic const&) &&;
 
   /*
    * Like 'at', above, except it returns either a pointer to the contained
@@ -392,8 +397,9 @@ public:
    *
    * These functions do not invalidate iterators.
    */
-  dynamic&       operator[](dynamic const&);
-  dynamic const& operator[](dynamic const&) const;
+  dynamic&       operator[](dynamic const&) &;
+  dynamic const& operator[](dynamic const&) const&;
+  dynamic        operator[](dynamic const&) &&;
 
   /*
    * Only defined for objects, throws TypeError otherwise.
@@ -404,8 +410,10 @@ public:
    * a reference to the existing value if present, the new value otherwise.
    */
   dynamic
-  getDefault(const dynamic& k, const dynamic& v = dynamic::object) const;
-  dynamic&& getDefault(const dynamic& k, dynamic&& v) const;
+  getDefault(const dynamic& k, const dynamic& v = dynamic::object) const&;
+  dynamic getDefault(const dynamic& k, dynamic&& v) const&;
+  dynamic getDefault(const dynamic& k, const dynamic& v = dynamic::object) &&;
+  dynamic getDefault(const dynamic& k, dynamic&& v) &&;
   template<class K, class V = dynamic>
   dynamic& setDefault(K&& k, V&& v = dynamic::object);
 

diff --git a/folly/test/DynamicTest.cpp b/folly/test/DynamicTest.cpp
index 184d57f451e442f4d335d0c824f7470e32048b3a..7ba744fb3a55351721a9c44ff9e88cc322326895 100644 (file)
--- a/folly/test/DynamicTest.cpp
+++ b/folly/test/DynamicTest.cpp
@@ -308,6 +308,124 @@ TEST(Dynamic, Assignment) {
   }
 }
 
+folly::fbstring make_long_string() {
+  return folly::fbstring(100, 'a');
+}
+
+TEST(Dynamic, GetDefault) {
+  const auto s = make_long_string();
+  dynamic ds(s);
+  dynamic tmp(s);
+  dynamic d1 = dynamic::object("key1", s);
+  dynamic d2 = dynamic::object("key2", s);
+  dynamic d3 = dynamic::object("key3", s);
+  dynamic d4 = dynamic::object("key4", s);
+  // lvalue - lvalue
+  dynamic ayy("ayy");
+  EXPECT_EQ(ds, d1.getDefault("key1", ayy));
+  EXPECT_EQ(ds, d1.getDefault("key1", ayy));
+  EXPECT_EQ(ds, d1.getDefault("not-a-key", tmp));
+  EXPECT_EQ(ds, tmp);
+  // lvalue - rvalue
+  EXPECT_EQ(ds, d1.getDefault("key1", "ayy"));
+  EXPECT_EQ(ds, d1.getDefault("key1", "ayy"));
+  EXPECT_EQ(ds, d1.getDefault("not-a-key", std::move(tmp)));
+  EXPECT_NE(ds, tmp);
+  // rvalue - lvalue
+  tmp = s;
+  EXPECT_EQ(ds, std::move(d1).getDefault("key1", ayy));
+  EXPECT_NE(ds, d1["key1"]);
+  EXPECT_EQ(ds, std::move(d2).getDefault("not-a-key", tmp));
+  EXPECT_EQ(dynamic(dynamic::object("key2", s)), d2);
+  EXPECT_EQ(ds, tmp);
+  // rvalue - rvalue
+  EXPECT_EQ(ds, std::move(d3).getDefault("key3", std::move(tmp)));
+  EXPECT_NE(ds, d3["key3"]);
+  EXPECT_EQ(ds, tmp);
+  EXPECT_EQ(ds, std::move(d4).getDefault("not-a-key", std::move(tmp)));
+  EXPECT_EQ(dynamic(dynamic::object("key4", s)), d4);
+  EXPECT_NE(ds, tmp);
+}
+
+TEST(Dynamic, GetString) {
+  const dynamic c(make_long_string());
+  dynamic d(make_long_string());
+  dynamic m(make_long_string());
+
+  auto s = make_long_string();
+
+  EXPECT_EQ(s, c.getString());
+  EXPECT_EQ(s, c.getString());
+
+  d.getString() += " hello";
+  EXPECT_EQ(s + " hello", d.getString());
+  EXPECT_EQ(s + " hello", d.getString());
+
+  EXPECT_EQ(s, std::move(m).getString());
+  EXPECT_NE(dynamic(s), m);
+}
+
+TEST(Dynamic, GetSmallThings) {
+  const dynamic cint(5);
+  const dynamic cdouble(5.0);
+  const dynamic cbool(true);
+  dynamic dint(5);
+  dynamic ddouble(5.0);
+  dynamic dbool(true);
+  dynamic mint(5);
+  dynamic mdouble(5.0);
+  dynamic mbool(true);
+
+  EXPECT_EQ(5, cint.getInt());
+  dint.getInt() = 6;
+  EXPECT_EQ(6, dint.getInt());
+  EXPECT_EQ(5, std::move(mint).getInt());
+
+  EXPECT_EQ(5.0, cdouble.getDouble());
+  ddouble.getDouble() = 6.0;
+  EXPECT_EQ(6.0, ddouble.getDouble());
+  EXPECT_EQ(5.0, std::move(mdouble).getDouble());
+
+  EXPECT_EQ(true, cbool.getBool());
+  dbool.getBool() = false;
+  EXPECT_FALSE(dbool.getBool());
+  EXPECT_EQ(true, std::move(mbool).getBool());
+}
+
+TEST(Dynamic, At) {
+  const dynamic cd = dynamic::object("key1", make_long_string());
+  dynamic dd = dynamic::object("key1", make_long_string());
+  dynamic md = dynamic::object("key1", make_long_string());
+
+  dynamic ds(make_long_string());
+  EXPECT_EQ(ds, cd.at("key1"));
+  EXPECT_EQ(ds, cd.at("key1"));
+
+  dd.at("key1").getString() += " hello";
+  EXPECT_EQ(dynamic(make_long_string() + " hello"), dd.at("key1"));
+  EXPECT_EQ(dynamic(make_long_string() + " hello"), dd.at("key1"));
+
+  EXPECT_EQ(ds, std::move(md).at("key1"));
+  EXPECT_NE(ds, md.at("key1"));
+}
+
+TEST(Dynamic, Brackets) {
+  const dynamic cd = dynamic::object("key1", make_long_string());
+  dynamic dd = dynamic::object("key1", make_long_string());
+  dynamic md = dynamic::object("key1", make_long_string());
+
+  dynamic ds(make_long_string());
+  EXPECT_EQ(ds, cd["key1"]);
+  EXPECT_EQ(ds, cd["key1"]);
+
+  dd["key1"].getString() += " hello";
+  EXPECT_EQ(dynamic(make_long_string() + " hello"), dd["key1"]);
+  EXPECT_EQ(dynamic(make_long_string() + " hello"), dd["key1"]);
+
+  EXPECT_EQ(ds, std::move(md)["key1"]);
+  EXPECT_NE(ds, md["key1"]);
+}
+
 int main(int argc, char** argv) {
   testing::InitGoogleTest(&argc, argv);
   gflags::ParseCommandLineFlags(&argc, &argv, true);