Fix issue#1275

[dota17]

Fix issue #1275 : add an overload of function value(key, default_value).

---

Pull request checklist

Read the Contribution Guidelines for detailed information.

• Changes are described in the pull request, or an existing issue is referenced.
• The test suite compiles and runs without error.
• Code coverage is 100%. Test cases can be added by editing the test suite.
• The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

[coveralls]

Coverage remained the same at 100.0% when pulling 4a6c68c on dota17:issue#1275 into 27f5a6e on nlohmann:develop.

[FrancoisChabot]

Reading through #1275, my impression is that the consensus was that moving out the value from the json object was deemed too surprising for users.

That aside, value() being a either a pure accessor or mutator depending on context sounds like a recipe for disaster.

So in my mind, this PR should either:

1. Name the new function a different name than an existing function, to reflect the fact that it does something else.
2. Be an update to the existing function to make it move out the held value (which would not be backwards compatible, so it would be a 4.0.0 change)
3. Be an update to the existing function that simply expands the range of legal values for the default_value argument.

[dota17]

@FrancoisChabot thank your advices! I think the first point is better, cause the second one will wait until the next major release, that will be a very long time. And the third one, I think it isn't the thing about the range of the default value.
But right now, I haven't got a new function name, I saw someone proposed to name pop_value(),
how do you think?

[FrancoisChabot]

Pedantically, I think this PR should ideally be 3), since that's what #1275 is about. 1) is practically a new feature in of itself.

But honestly, I'm personally not a huge fan of 1). I do not like extra API surface for the sake of minor QoL features in general. I can do std::move(json_obj.get_ref<std::string>()), and that's good enough for me. I've got enough cognitive load to deal with as it is.

[nlohmann]

I'm against breaking the API, and I am not really convinced that adding a new function is really helpful. This is a very small corner case, and it is possible to implement it via the public API.

[dota17]

Ok, I will not add a new function to break the API, but only focus on 3).
But I am confused with the meaning of expanding the range of the default_value.

[FrancoisChabot]

It's just an awkward way of saying that the function would now accept R-Value references when appropriate.

[dota17]

@nlohmann @FrancoisChabot Are you satisfied with the last changes?

[FrancoisChabot]

This looks fine to me.

[nlohmann]

Looks good to me.

[nlohmann]

Thanks!

[lethe555]

just break my code

json j{{"x", "test"}};
std::string defval = "default value";
auto val = j.value("x",defval);
auto val2 = j.value("y",defval);

json.hpp:19714:25: warning: returning reference to temporary [-Wreturn-local-addr]
return *it;

[gregmarr]

@lethe555 Which line is 19714? Assuming it's the return *it, you haven't shown us what it is.

[lethe555]

@gregmarr

json j{{"x", "test"}};
std::string defval = "default value";
auto val = j.value("x",defval);
auto val2 = j.value("y",defval);

error: no matching function for call to 'nlohmann::basic_json<>::value(const char [2], std::string&)'
auto val = j.value("x", defval);

now default_value seems can't be a lvalue , but use a lvalue as default value is very common case

[nlohmann]

I can confirm that the code in #2181 (comment) does not compile with the latest develop version. Reason is this PR. I shall revert the changes and add a regression test. Then, we can decide how to properly address #1275.

[nlohmann]

Done.

[gregmarr]

I think you might be able to do this same basic PR but using std::forward instead of std::move.

[nlohmann]

I can't get it to run:

../test/src/unit-regression.cpp:1954:22: error: no matching member function for call to 'value'
        auto val = j.value("x", defval);
                   ~~^~~~~
../include/nlohmann/json.hpp:3810:14: note: candidate function not viable: no known conversion from 'std::string' (aka 'basic_string<char, char_traits<char>, allocator<char> >') to 'const char *' for 2nd argument
    string_t value(const typename object_t::key_type& key, const char* default_value) const
             ^
../include/nlohmann/json.hpp:3884:14: note: candidate function not viable: no known conversion from 'const char [2]' to 'const nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::json_pointer' (aka 'const json_pointer<nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > >') for 1st argument
    string_t value(const json_pointer& ptr, const char* default_value) const
             ^
../include/nlohmann/json.hpp:3788:15: note: candidate template ignored: requirement 'std::is_convertible<nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > &>::value' was not satisfied [with ValueType = std::__1::basic_string<char> &]
    ValueType value(const typename object_t::key_type& key, ValueType&& default_value) const
              ^
../include/nlohmann/json.hpp:3860:15: note: candidate template ignored: requirement 'std::is_convertible<nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > &>::value' was not satisfied [with ValueType = std::__1::basic_string<char> &]
    ValueType value(const json_pointer& ptr, ValueType&& default_value) const

[dota17]

@nlohmann
Please see dota17#18, I made a new PR in my fork repo for this issue.
And from CI log, the test SECTION("PR #2181 - regression bug with lvalue") had no error for new PR.
I just added back the original method and then there would be two methods:

ValueType value(const typename object_t::key_type& key, const ValueType& default_value) const //  added back
...
ValueType value(const typename object_t::key_type& key, ValueType && default_value) const

If you think it works, I would push it to the official repo.

[nlohmann]

That would be great.