From 117385fb60e5acff59f2182fce6a259cf36f4820 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Fri, 31 Jul 2015 09:57:00 -0700 Subject: [PATCH] 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 --- folly/dynamic-inl.h | 33 +++++++---- folly/dynamic.cpp | 32 +++++++--- folly/dynamic.h | 36 ++++++----- folly/test/DynamicTest.cpp | 118 +++++++++++++++++++++++++++++++++++++ 4 files changed, 188 insertions(+), 31 deletions(-) diff --git a/folly/dynamic-inl.h b/folly/dynamic-inl.h index 6b42fab2..6e2fd62a 100644 --- a/folly/dynamic-inl.h +++ b/folly/dynamic-inl.h @@ -384,15 +384,20 @@ inline double dynamic::asDouble() const { return asImpl(); } inline int64_t dynamic::asInt() const { return asImpl(); } inline bool dynamic::asBool() const { return asImpl(); } -inline const fbstring& dynamic::getString() const { return get(); } -inline double dynamic::getDouble() const { return get(); } -inline int64_t dynamic::getInt() const { return get(); } -inline bool dynamic::getBool() const { return get(); } +inline const fbstring& dynamic::getString() const& { return get(); } +inline double dynamic::getDouble() const& { return get(); } +inline int64_t dynamic::getInt() const& { return get(); } +inline bool dynamic::getBool() const& { return get(); } -inline fbstring& dynamic::getString() { return get(); } -inline double& dynamic::getDouble() { return get(); } -inline int64_t& dynamic::getInt() { return get(); } -inline bool& dynamic::getBool() { return get(); } +inline fbstring& dynamic::getString() & { return get(); } +inline double& dynamic::getDouble() & { return get(); } +inline int64_t& dynamic::getInt() & { return get(); } +inline bool& dynamic::getBool() & { return get(); } + +inline fbstring dynamic::getString() && { return std::move(get()); } +inline double dynamic::getDouble() && { return get(); } +inline int64_t dynamic::getInt() && { return get(); } +inline bool dynamic::getBool() && { return get(); } inline const char* dynamic::data() const& { return get().data(); } inline const char* dynamic::c_str() const& { return get().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 inline dynamic& dynamic::setDefault(K&& k, V&& v) { auto& obj = get(); return obj.insert(std::make_pair(std::forward(k), @@ -474,10 +483,14 @@ inline dynamic* dynamic::get_ptr(dynamic const& idx) & { return const_cast(const_cast(this)->get_ptr(idx)); } -inline dynamic& dynamic::at(dynamic const& idx) { +inline dynamic& dynamic::at(dynamic const& idx) & { return const_cast(const_cast(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 f7be57e8..d743bb22 100644 --- 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(); 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(); 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(); + 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(); + 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()) { if (!idx.isInt()) { throw TypeError("int64", idx.type()); diff --git a/folly/dynamic.h b/folly/dynamic.h index 35ef9b5c..bea7fc44 100644 --- 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 dynamic& setDefault(K&& k, V&& v = dynamic::object); diff --git a/folly/test/DynamicTest.cpp b/folly/test/DynamicTest.cpp index 184d57f4..7ba744fb 100644 --- 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); -- 2.34.1