Re: [review][json] Boost.JSON review

From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2020-09-22 18:28:54

On Tue, Sep 22, 2020 at 2:59 AM Ruben Perez via Boost
<boost_at_[hidden]> wrote:
> My recommendation is to ACCEPT Boost.JSON into Boost.

Thank you for looking at the library and writing up a review.

>   1. The memory management model is extremely simple. I wonder if
>      storage_ptr could be moved somewhere else long term so other
>      libraries can use it.

This is something that has come up a few times, and I think it is
worth the review manager making a special note of it. For now I am
content to simply have an initial release of Boost.JSON where the
storage_ptr system is part of the library (which will be necessary for
standalone anyway). And we can see how the community feels about it,
work out any possible kinks.

After some time for the thing to mature, we can measure support for
proposing it as its own library, or perhaps making it part of another
library.

> I miss some way of accessing json::value from generic code,
> but I understand visitation is currently in progress. As a user
> I would also be satisfied with generic versions of get_xxx,
> is_xxx and so on:

I'm open to it, but could you open an issue describing exactly how
this would work? What types are permissible to pass to `get<>`? Does
it have to be a member function (because you have to put the word
template there in generic contexts). This is worth exploring.

> I also miss some way of determining the location (line and column)
> of parse errors. I don't know how this feature would interact with
> performance though. Nice to have but not required for me.

Well, basic_parser is written so that the return value from write
(soon, write_some) is exactly the number of valid characters before a
parse error occurred. So you can find out precisely where in the
buffer the problem happened. It would be pretty easy to write your own
function which uses parser and also communicates this information
somehow (perhaps an out-param).

> I would have also liked `json::kind` be streamable, as it
> makes logging/debugging easier in some contexts. Nothing I can't
> live without, though.

We can do that. Track it here:

<https://github.com/CPPAlliance/json/issues/405>

> Finally, I would also appreciate `json::string` have a
> straightforward way to be converted to `std::string` (e.g.
> by having a `to_std_string()` member function).

hmmm... I'm not opposed to this in principle, but then people with no
need for this conversion would be including <string>. We are
unfortunately including that already, but I have plans to not include
it. Track it here:

<https://github.com/CPPAlliance/json/issues/406>

> I would say the main point of improvement would be `basic_parser`,
> as I had a hard time understanding when some of the callbacks
> get called, especially the `on_xxx_part` ones. It would be also good
> to know if accessing the `basic_parser` state (e.g. `depth()`) is allowed
> from within the handler. And if it is

> maybe passing the parser instance
> at the beginning of the parse could be useful (e.g. in the
> `on_document_begin`,

If we do this, the handler would receive `basic_parser<Handler>
const&` as the first argument on construction. Yes you can access
`depth()`. Track the issue here:

<https://github.com/CPPAlliance/json/issues/408>

> I've noted that streaming a `json::string` outputs the string
> within double quotes. It surprised me at first, as I was working
> with the 'json::string equals std::string' mentality. I guess this
> behavior makes sense, but I would make a note in the 'Using strings'
> section.

Yes, track that here:

<https://github.com/CPPAlliance/json/issues/409>

> 'See also' section for
> https://master.json.cpp.al/json/ref/boost__json__value_from_tag.html
> renders functions without separation, looks strange.

Typo :) Track here:

<https://github.com/CPPAlliance/json/issues/410>

> I would make a note in the Value conversion mechanism referencing
> P1895 on `tag_invoke` so users who don't know this paper don't
> stare at the name wondering 'why?'

Agree! Track:

<https://github.com/CPPAlliance/json/issues/411>

Regards