[JSON][review] Some early feedback

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2020-09-10 13:34:29

Hi Everyone,
The formal review of Boost.JSON has not started yet, but I thought I could
offer some feedback from my playing with the library earlier in the
process.

Documentation
---------------

Very well written. It makes it easy to grasp quickly the spirit and the
potential of the library, and the reference section makes it easy to
navigate through the features and their interfaces.

Some minor issues:
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++.

Design.
-----------

It is generally sound, and well explained in the rationale. Here are a
couple of minor remarks.

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.

2. Why is json::value Semiregular rather than Regular? Equality comparison
has a natural interpretation and application for checking if the produced
JSON looks exactly as expected, or for checking if some two subtrees are
the same. Is it because of efficiency issues with json::object comparison?

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.

3.1. It is not the first library reviewed his year to have a custom type
with std::string-compatible interface. It looks like the idea of having a
"vocabulary" string type is unrealistic. If this should be the case, maybe
we need a concept specifying what a string-compatible type should provide.
It is obvious that json::string does not literally provide the same
interface as std::string. Nor does anyone expect 100% compatibility.

3.2. Docs say that json::string is like std::string *except for* a number
of things. For instance it does not provide the controversial assignment
from `char`. Given that you are "fixing" the shortcomings of std::string,
maybe it is an opportunity to fix other things in it also. For instance,
std::string has been criticised for having too big an interface. That it
mixes an iterator-based interface with an indexed-based one. Myb functions
like `find_first_not_of()` are not necessary, given that users can use all
sorts of iterator-based algorithms?

3.3. The rationale for json::string says that inisializer_list constructor
is unnecessary, because string literal can always be used instead. This
does not take into account that initializer_list does not necessarily
operate on constant values. There is no concise substitute for:

```
std::string arrange(char a, char b, char c)
{
  return {c, a, b, c};
}
```

I would not miss this construction much, I guess, but I would expect this
to be mentioned in the rationale.

4. One thing one often expects of a JSON library is to be able to
pretty-print a JSON document. The docs give an example how to do it, but
the library does not provide this feature out of the box. Is
pretty-printing not compatible with the design goals of the library?

Implementation
--------------

I didn't look much at it yet, but here is something I found.

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.

2. The following piece of code

```
json::value v = 50.0;
std::cout << json::serialize(v);
```

oututs:
5E1

which is technically compliant with JSON specification, but quite
surprising. Is there a reason for this? Can serialization be controlled to
output 50.0 or 50?

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.
Am I missing something?

4. I really appreciate the ability to use the library in header-only mode
with <boost/json/src.hpp>. This makes library testing much simpler.

Regards,
&rzej;