Re: [JSON][review] Some early feedback

From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2020-09-10 15:00:38

On Thu, Sep 10, 2020 at 6:34 AM Andrzej Krzemienski <akrzemi1_at_[hidden]> wrote:
> I thought I could offer some feedback from my playing with the library earlier in the process.

Thank you for spending time to look at the library and provide feedback.

> 1. Section "Using Numbers" is missing content.
> 2. Description of storage_ptr's destructor and move constructor is missing noexcept.
> 3. Docs in a couple of places call `storage_ptr` a "smart pointer container".
> Is it intentional? "Container" has a very specific meaning in C++.

I still have a couple of passes to make on the documentation.

With respect to "smart pointer container" - I thought that was the
correct term? Although, now that I look on cppreference it calls
shared_ptr simply a "smart pointer." I guess "smart pointer" is the
right term, as storage_ptr most closely resembles shared_ptr in its
operation. Should I apply this change throughout?

> 1. The usage of `nullptr` for null values.

Well, as you said I could add json::null_t and a constant json::null.
It isn't clear if that is better. With this library I have tried to
avoid a proliferation of types, to keep things simple.

> 2. Why is json::value Semiregular rather than Regular?...
> Is it because of efficiency issues with json::object comparison?

Yes. The question of object comparison is certainly in-scope for this
library. However I do not plan to solve it until after the library has
gotten more real-world use. Very likely I will add equality comparison
as an example first, to get field experience and feedback.

> 3. The library is using a custom string type. We get a rationale for this, and I accept it. But there are still some concerns about it that I would like to mention.
>
> It is obvious that json::string does not literally provide the same interface as std::string.
> Nor does anyone expect 100% compatibility.

It is pretty close though, I look at the declaration for std::string
on cppreference to match as much of it as I could, that made sense to
do so.

> Myb functions like `find_first_not_of()` are not necessary, given that
> users can use all sorts of iterator-based algorithms?

Maybe. In fact, the implementations of these just call the
corresponding function of the string view:

<https://github.com/CPPAlliance/json/blob/07214ad235b0ea43a1ef49a11ee3807d8198cebe/include/boost/json/string.hpp#L2540>

There are pros and cons for including these. The pros are, they are
easily implemented and obviously correct. Code that currently uses
std::string is more likely to compile if the type is changed to
json::string. The cons, as you pointed out, larger interface surface,
more ways of doing the same thing. I'm not sure what the correct
answer is.

> 4. One thing one often expects of a JSON library is to be able to pretty-print...
> but the library does not provide this feature out of the box. Is pretty-printing
> not compatible with the design goals of the library?

The serializer is tuned first and foremost for performance and ease of
use. We could do for the serializer what we did for the parser, which
is to support options such as pretty printing, line breaks,
indentation, and so on, but that would increase the size of the
emitted code. More likely we could add a new serializer that supports
pretty printing and other types of output options, at the expense of
reduced performance (especially anything to do with floating-point
formatting). However for the first release, pretty-printing is not
something that we are ready to make public.

> 1. `json::string_view` is convertible from `std::stirng`, but it is not convertible from `std::string_view`.
>  I think there is no harm in enabling this conversion, and it seems useful.

We don't have control over that, since `json::string_view` is just a
type alias for `boost::string_view`.

> 5E1...Is there a reason for this? Can serialization be controlled to output 50.0 or 50?

For the first release the goal is to have something that works and is
reasonably fast. Conversion from floating point to string is
difficult, and will be a topic of improvement for at least the first
few versions of the library. We have a number of alternative
algorithms to evaluate in the pipeline.

> 3. It is confusing for me to see member function serializer::read.
> I would think that the action that a serializer does is *write* rather than read.

Yeah... well you read from a socket into your buffer. And you write to
a socket from your const buffer.

Thanks