Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding additional push_back/operator+= rvalue overloads for JSON object #657

Closed
himikof opened this issue Jul 14, 2017 · 3 comments
Closed

Comments

@himikof
Copy link
Contributor

himikof commented Jul 14, 2017

I propose adding the following three overloads to basic_json:

void push_back(typename object_t::value_type&& val);
value_type& operator+=(typename object_t::value_type&& val) &;
value_type&& operator+=(typename object_t::value_type&& val) &&;

These are symmetric to existing const typename object_t::value_type& overloads,
they bring the interface more in line with std::map (there are both insert(value_type const&) and insert(value_type &&) methods). The third overload allows rvalue json object to remain a temporary after operator+=.

Most importantly, these should allow for this simple code perform without copying:

return json {
   {"type", "Polygon"},
} += json::object_t::value_type {"coordinates", std::move(coordinates)};

Please note that this code is not equivalent to

return json {
   {"type", "Polygon"},
   {"coordinates", std::move(coordinates)},
};

because sadly in C++ std::initializer_list breaks perfect forwarding, and (possibly large) subobject is silently copied in this case.

Without these overloads, I think that this could only be expressed (without runtime overhead) by

json result = {
   {"type", "Polygon"},
};
result.emplace("coordinates", std::move(coordinates));
return result;

which is not very convenient.

Another alternative is to provide an explicit chainable method instead of (ab-)using operator+=. This could lead to cleaner code like:

return json {
   {"type", "Polygon"},
}.method_name("coordinates", std::move(coordinates));

This method should return an rvalue reference when called on an rvalue object, too.

@nlohmann
Copy link
Owner

This seems like a reasonable addition. PRs welcome!

@himikof
Copy link
Contributor Author

himikof commented Jul 30, 2017

Most of proposed additions (and probably all of them) are unnecessary since #663 landed and only complicate the API, therefore closing.

@himikof himikof closed this as completed Jul 30, 2017
@nlohmann
Copy link
Owner

Thanks for checking back!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants