On 10/09/2020 14:34, Andrzej Krzemienski via Boost wrote:

1. The usage of `nullptr` for null values. `nullptr` has been introduced to mean "a pointer with null value" and is only used in the standard library for smart pointers, with an ignoble exception of `std::function<>`. Using it for `json::value` is not an obvious choice for me. On the other hand, we do not have a universal type representing a missing value. A library could provide its own `json::null`, although I am not sure if this is an ideal solution either.

Personally speaking, I find the use of nullptr to mean "initialise with null bits" fine. So, for example, you might have this: struct Foo { ... constexpr Foo() {} // uninitialised contents constructor constexpr explicit Foo(std::nullptr_t) {} // initialise contents to default values }; That's the fullest extent that I would overload the use of nullptr however. Personally speaking, if json::value needs a null state, a default constructed instance seems the right approach. However if json::value is a variant, then monostate seems a better choice. Niall

On Thu, Sep 10, 2020 at 6:34 AM Andrzej Krzemienski 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/07214ad235b0ea43a1ef49a11ee3807d819... 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