if_contains currently returns value*, but it'd be better if it returned optional<value&>

[brevzin]

Currently, json::object provides if_contains that returns a json::value* (or json::value const*). This is a useful API, definitely more ergonomic than having to call find() and check against end().

However, it could be more ergonomic still. boost::optional actually supports references, and boost::optional<value&> is a much more appropriate type here than value*. For simple uses, they're the same:

if (auto value = obj.if_contains(key)) {
    use(*value);
}

But returning optional allows you to chain further operations and more conveniently access that underlying value. Like, say:

int val = obj.if_contains(key)
    .map([](json::value const& v){ return v.to_number<int>(); })
    .value_or(-1);

optional enables this approach, whereas with pointers you'd have to do something like:

int val = [&]{
    auto p = obj.if_contains(key);
    return p ? p->to_number<int>() : -1;
}();

This pretty quickly gets more and more clunky. And then here's a bunch more reasons to prefer optional<value&> here.

In short, even if you don't care much for the continuation style, optional<value&> supports all the existing valid uses of value* that exist today, doesn't support any of the existing invalid uses (like delete-ing the pointer, indexing the pointer, incrementing the pointer, ...), and supports more useful operations that do regularly come up (like map, and_then, value_or, ...)

[vinniefalco]

Well it would have to return boost::optional since this library has to work in C++11, is this what you want?

[brevzin]

Yeah, it also would have to be boost::optional since std::optional doesn't support references.

[grisumbras]

I'm not opposed, but there is the issue of invalidating user code. optional<T&> doesn't implicitly convert to T*. So, changing the return type would break every user who didn't use auto.

[brevzin]

Theoretically you could return a type like:

namespace boost::json {
    template <typename T>
    struct optional : boost::optional<T&> {
        operator T*() const;
    };
}

which would preserve the if (json::value* v = o.if_contains(k)) use-cases in addition to the auto ones, but would still break on the auto* ones and some other more complicated shenanigans.

But I dunno, seems worth breaking that code to give people a better API.

---

Or you could just provide this other version under a different name. Like try_at() (since this function seems more closely related to at(), which returns a T&, than contains(), which returns bool). That adds a bit of redundancy, but definitely won't break any users.

[vinniefalco]

I like better ergonomics. Dmitry can you perhaps do an internet and a smart GitHub search to get an idea of how many people are using it?

[grisumbras]

I can try

[grisumbras]

I've discovered these projects that currently use JSON's if_contains, and would be broken by the change:

• https://github.com/allesafe-com/beng-proxy/
• https://github.com/CdecPGL/PlanetaMatchMaker/
• https://github.com/CM4all/libcommon/
• https://github.com/DyXel/Multirole/
• https://github.com/gmarino2048/circuit-simulator/
• https://github.com/guard3/GreekBot/
• https://github.com/nn6n/beng-proxy/
• https://github.com/RHVoice/RHVoice/
• https://github.com/SOURC3XYZ/sourc3/
• https://github.com/sptrakesh/config-db
• https://github.com/StoneXLabs/stonex-cms-amqp-test-engine
• https://github.com/XRPLF/clio/

Ironically, most use auto, but still require a pointer (via auto*). Many of these projects were fairly recently updated.

[kamrann]

As someone who uses this library but in general is trying to minimize boost dependencies, can I ask for such a change to allow for opt-in/opt-out through a preprocessor define or similar? Compile-time overhead is my main reason for this - I'm using std::optional throughout my project and don't want translation units to start pulling in both std::optional and boost::optional. But there's also the added possibility that boost.optional will bring in more transitive dependencies that weren't already in boost.json, bloating the build footprint regardless of compile times.

FWIW, I'll add that I have no issue with breaking changes. The library is still young so I'd agree that breaking changes are preferable to workarounds.

[grisumbras]

Dependency on Optional would also add dependency on Detail, Utility, Preprocessor, and Io. All fairly small libraries.

If we go with this, we will not add a config option to choose optional implementation. Things like that are a constant source of headache for both users and maintainers, because people keep building libraries with one configuration and linking to it with another, which get them linking errors they have no idea how to fix.

[nigels-com]

I've been broadly interested in using std::optional as an alternative to throw-happy paths such as boost::json::object::at.
I have two suggestions to add here.

1. In addition to pointer-returning if_contains, an equivalent std::optional returning optional.
2. The optional API is only available for C++17 onwards, C++11 code can continue using if_contains.

I had a change accepted to the cxxopts library along similar lines in
PR 421. That API now has a throwing as and a non-throwing as_optional. So the boost::json::object name could be at for throwing and at_optional for non-throwing perhaps?

[nigels-com]

There is a call for GSOC '24 mentors on the boost mailing list. If this sort of "C++17 enrichment" project for boost::json is of interest to the maintainers, I'd be willing to propose, mentor and co-ordinate the work with input from the boost::json people. I've also thought about std::expected for the error-code path in boost::json as possibly being a nice thing in C++23 mode.

[nigels-com]

While I'm here rambling about std::optional I did also want to mention the CppCon 2023 talk by Peter Muldoon
Exceptionally Bad: The Misuse of Exceptions in C++ & How to Do Better which I did find informative in terms of reducing the use of C++ exceptions in cases like this.

[grisumbras]

I have a WIP branch (locally) that adds accessor functions that return system::result and not optional.

Why add? I decided that changing all of the pointer-returning functions would be too much of a change. Maybe it would have been a good idea initially (and Boost.URL did in fact go with it), but for JSON this ship has sailed.

Why system::result? It gives more information to the caller. And even if there's only one reason for a function to fail (e.g. array::at), usually you have multiple calls to accessor functions and bail out if you fail, and so it's just more convenient to get the error instead of a generic "not there". Peter also added a bunch of chaining operators to it. And I requested an even more powerful invoke function that is supposed to work as n-ary transform, and n-ary bind, and (semantically) does the monadic lifting of arguments for you. So, combining results will be very easy. Oh, and also we already depend on System, so we won't even add any new dependencies.

[nigels-com]

I would definitely make use of an API using boost::system::result. Ideally I might like C++23 std::expected a little more, but system::result is workable here and now. It's std::optional enough for me.

[vinniefalco]

can I ask for such a change to allow for opt-in/opt-out through a preprocessor define

No. The problem with such macros is that it effectively creates two different and API-incompatible libraries.

[grisumbras]

#941 adds accessor functions that return system::result<T&>. I decided against changing existing functions, because I considered that change to be too disruptive. So, closing as "not planned".