Static method invites inadvertent logic error.

[madmongo1]

• What is the issue you have?

I recenly wrote code like this:

json jbody;
jbody.parse(upstream_response_.body());
auth_object_.set_user_id(jbody.at("user_id").get<std::string>());

It compiled fine, but subsequent tests failed (i.e. the expected result was not realised at runtime).

2 hours later I realised my mistake. json::parse is a static method that returns a json object. But the compiler was not able to help me discover my mistake early.

Suggestions:

nodiscard

add a [[nodiscard]] directive to the function when detected that the compiler supports it.

deprecate confusing interface

possibly deprecate static json json::parse(...)

deduce json type through ADL

possibly provide an ADL free function overload suite such that the user may call:

json target;
json_parse(target, source, ...); // returns error state or possibly throws

If users wish to have a parse that returns a newly constructed json object, this can be self provided, example:

template<class Source>
auto json parse_to_json(Source&& source) -> nlohmann::json
{
    nlohmann::json result;
    json_parse(result, source);    // located through ADL
    return result;
}

More complex user-defined functions may want to deduce the allocator from the source, making it easier to use the library with custom allocators such as arenas, this improving performance.

E.g.

template<class Ch, Class ChT, class Alloc>
auto json parse_to_json(std::basic_string<Ch, ChT, Alloc> const& source)
{
    // note: nlohmann::value_base might be a new type, used to provide a rebind
    // to internal maps, vectors, etc
    using allocator_type = typename std::allocator_traits<Alloc>::template rebind_alloc<nlohmann::value_base>;

    // note: providing custom allocator as an argument.
    nlohmann::basic_json<....., allocator_type> result(source.get_allocator());
    json_parse(result, source);    // located through ADL. Uses internal allocator
    return result;
}

• Please describe the steps to reproduce the issue. Can you provide a small but working code example?

see above

• What is the expected behavior?

I intuitively expected parse to be a member function which would mutate jbody.

• And what is the actual behavior instead?

The return value was silently discarded.

• Which compiler and operating system are you using? Is it a supported compiler?

clang, gcc8, fedora29, osx.

• Did you use a released version of the library or the version from the develop branch?

released

• If you experience a compilation error: can you compile and run the unit tests?

N/A

[nlohmann]

Some thoughts on this:

• [[nodiscard]] is not supported until C++17. This library targets C++11.
• deprecating the function is not an option: it is part of the library since 1.0.0, and there was never a report of confusing behavior
• I personally find the idea of a member function to parse unintuitive, because it would mean I first have to create a value and then "initialize" it with a parsed value

I did not fully grasp the idea with the allocator. Unfortunately, allocator support is very weak (see #161 for an early issue). Maybe this could be a base for an additional function. However, using the SAX parser interface would also allow to create a JSON value without having the parser allocate it. See the documentation for more information.

[gregmarr]

I've seen linter checks for calling a static function on an object.

I think we can safely put [[nodiscard]] on things even though it has no meaning in C++11. Compilers are supposed to ignore unknown attributes.

[madmongo1]

Thanks for the response.

I would imagine it's possible to deduce whether [[nodiscard]] (or compiler-specific attributes such as __attribute__((nodiscard)) on gcc) are available in the same way that boost does. i.e. by querying whether known compiler macros are defined and what their values are. But this is a detail. It seems to me that a user will never want to discard the result of this static method, so if there could be protection there, there ought to be.

It would be nice to have a DOM built as it is now, but in a memory buffer defined by a custom allocator. This could improve performance for 'JSON views' dramatically. But I think this is a subject for another PR. I'll have a think about it.

[madmongo1]

quick example: https://godbolt.org/z/-tLO1K

[nlohmann]

@gregmarr @madmongo1 Thanks for the hint wrt. nodiscard! This would be definitely a good addition - also for some other functions maybe.

@madmongo1 I'm looking forward to your ideas on how to improve the memory management!

[FrancoisChabot]

The right way to do this would probably be to use __has_cpp_attribute() if present.

#define JSON_NODISCARD

#if defined(__has_cpp_attribute)
#if __has_cpp_attribute(nodiscard)
#undef JSON_NODISCARD
#define JSON_NODISCARD [[nodiscard]]
#endif
#endif

This way, modern compilers compiling code with C++11 flags are still allowed to apply the attribute.

[madmongo1]

@gregmarr @madmongo1 Thanks for the hint wrt. nodiscard! This would be definitely a good addition - also for some other functions maybe.

@madmongo1 I'm looking forward to your ideas on how to improve the memory management!

I am thinking about it. It's not trivial because we'd want to propagate the rebound allocator to the vector, map and string templates.

[FrancoisChabot]

@madmongo1

I am thinking about it. It's not trivial because we'd want to propagate the rebound allocator to the vector, map and string templates.

Maybe I'm missing something (and this is definitely out of the scope of this issue), but can't you get where you want memory-wise pretty easily already?

using json = nlohmann::basic_json<std::map, 
  std::vector, 
  std::basic_string<char, std::char_traits<char>,My_allocator<char>>, 
  bool,
  std::int64_t,
  std::uint64_t,
  double, 
  My_allocator>;

[nlohmann]

@FrancoisChabot Please see an old discussion in #161 - I fear it is not so easy.

[nlohmann]

Fixed with e89c946

[madmongo1]

On Sun, 20 Jan 2019, 21:43 Niels Lohmann ***@***.*** wrote: Fixed with e89c946 <e89c946>

Awesome. Thank you. —

…

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1433 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE7syeYvDWgCI8mTYFfDG-zdQt5Fxe9dks5vFIB4gaJpZM4aAldz> .