Suggestion to improve value() accessors with respect to move semantics

[murderotica]

Here's a couple suggestions that I think might be worth adding to improve the usage of value() function:

1. Use "universal reference" && instead of const& for default_value to lift a problem that const ValueType& default_value inhibits move when default_value is temporary/r-value:

Before:

template<class ValueType, typename std::enable_if<
                 std::is_convertible<basic_json_t, ValueType>::value, int>::type = 0>
    ValueType value(const typename object_t::key_type& key, const ValueType& default_value) const
    {
        // at only works for objects
        if (JSON_LIKELY(is_object()))
        {
            // if key is found, return value and given default value otherwise
            const auto it = find(key);
            if (it != end())
            {
                return *it;
            }

            return default_value;
        }

        JSON_THROW(type_error::create(306, "cannot use value() with " + std::string(type_name())));
}

After

template<class ValueType, typename std::enable_if<
                 std::is_convertible<basic_json_t, detail::uncvref_t<ValueType>>::value, int>::type = 0>
    detail::uncvref_t<ValueType> value(const typename object_t::key_type& key, ValueType&& default_value) const &
    {
        // at only works for objects
        if (JSON_LIKELY(is_object()))
        {
            // if key is found, return value and given default value otherwise
            const auto it = find(key);
            if (it != end())
            {
                return *it;
            }
            return std::forward<ValueType>(default_value);
        }

        JSON_THROW(type_error::create(306, "cannot use value() with " + std::string(type_name())));
    }

This will allow this:

json j{{"x", "test"}};
std::string defval = "default value";
auto val = j.value("y", std::move(defval) /* or a temporary constructed in-place */);

// defval is now empty (moved-from) and val contains "default value"

2. Provide r-value reference overload for value() to allow moving from JSON's values when JSON is temporary/r-value:

template<class ValueType, typename std::enable_if<
                 std::is_convertible<basic_json_t, detail::uncvref_t<ValueType>>::value, int>::type = 0>
    detail::uncvref_t<ValueType> value(const typename object_t::key_type& key, ValueType&& default_value) &&
    {
        // at only works for objects
        if (JSON_LIKELY(is_object()))
        {
            // if key is found, return value and given default value otherwise
            const auto it = find(key);
            if (it != end())
            {
                return std::move(it->template get_ref<ValueType&>());
            }

            return std::forward<ValueType>(default_value);
        }

        JSON_THROW(type_error::create(306, "cannot use value() with " + std::string(type_name())));
    }

This will make possible the following:

json j{{"x", "123"}};
auto val = std::move(j).value("x", std::string{"whatever"});

// j["x"] in now empty (moved-from) and val contains "123"

[murderotica]

One more thing: it may also be useful to provide r-value reference overload to the conversion operator ValueType() to allow moving elements when resulting type is not basic_json:

Currently, this doesn't do what one might expect:

json j{{"x", "123"}};
std::string val = std::move(j["x"]);
// oops, j["x"] still has the value "123" - we just copied stuff even though wanted to move
// we have to do this to make it work, cumbersome
std::string val = std::move(j["x"].get_ref<std::string&>());

[jaredgrubb]

+1 on the first point. Forwarding reference definitely makes sense there.

I don't agree on the second one. I could see an argument for it to return a "ValueType&&", but returning a "ValueType" with a side-effect of changing the underlying json object feels "surprising" to me (and I don't like surprises in interfaces :)).

[murderotica]

I don't agree on the second one. I could see an argument for it to return a "ValueType&&", but returning a "ValueType" with a side-effect of changing the underlying json object feels "surprising" to me (and I don't like surprises in interfaces :)).

You can't return ValueType&& since it won't do what I would expected it to after doing move(json).value(...) because the return type here is deduced at the same time as the type of default_value (that is, if ValueType is an l-value ref then you will return an l-value ref and copy will occur instead of move, what's more, is that once you will try to use and l-value ref as default_value you will not be able to perfom std::move() on the existing value, because you can't bind lvalue ref to rvalue). While return by value will actually work as expected. Remember that it is an r-value overload.

[jaredgrubb]

I wrote "ValueType&&" as shorthand, sorry for being imprecise. I meant that it really should be returning rvalue in all cases (aka, detail::uncvref_t<ValueType>&& value(..., ValueType&&) &&), but you're right that if ValueType&& == T const& then you have a binding problem.

You could solve this by adding additional overloads to trap those, but then you get very different behavior depending on what type your second argument is, and that is also a "surprising" API to me.

All in all, I like the spirit of your (2) suggestion, but I don't see a way to get all the benefits without adding surprises too. The idea that a function named "value(key,def)&&" has side effects is surprising to me as a code-maintainer. If you rename your function to "pop_value(key,def)&&" that returns value type (exactly as you proposed), then I am totally on board.

[nlohmann]

I like the original proposal from @murderotica. Could you provide a PR?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[nlohmann]

Reopened due to bug found. See #2181.

[nlohmann]

PR #2251 reverts #2181. Happy to see PRs to fix this issue once #2251 is merged.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.