[JSON][review] Some early feedback

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[JSON][review] Some early feedback

Boost - Dev mailing list
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;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [JSON][review] Some early feedback

Boost - Dev mailing list
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [JSON][review] Some early feedback

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Andrzej Krzemienski wrote:

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

Half of the existing JSON libraries map null to nullptr, it's pretty well
established and idiomatic at this point.

> 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?

s.read( buf ) writes data to the buffer you supply, just like fread writes
data to the buffer you supply. It's a bit disorienting initially, but does
make sense. You read from the serializer.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [JSON][review] Some early feedback

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Thu, Sep 10, 2020 at 6:34 AM Andrzej Krzemienski <[hidden email]> 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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [JSON][review] Some early feedback

Boost - Dev mailing list
>> 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.
We have other cases, e.g. boost::none(_t) for optional. But as in this
case it is the same as a default constructed value, why do we need it?
In C++11 (which this library targets) you could use `return {};` and the
like.
I guess it is fine though as JSON null and nullptr are close enough in
meaning
> s.read( buf ) writes data to the buffer you supply
I get the rationale. But reading that code I would expect that "s" reads
from "buf". So to me the behavior is confusing and hence error prone.
Could you explore alternatives?



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [JSON][review] Some early feedback

Boost - Dev mailing list
On Thu, Sep 10, 2020 at 8:15 AM Alexander Grund via Boost
<[hidden email]> wrote:
> > s.read( buf ) writes data to the buffer you supply
> I get the rationale. But reading that code I would expect that "s" reads
> from "buf". So to me the behavior is confusing and hence error prone.
> Could you explore alternatives?

I agree that there is some confusion, but I believe that confusion is
unavoidable. In other words no matter what the verb, it will always be
subject to interpretation both ways. The way I "break the tie" is
simply to follow the convention used in networking. You read from a
socket into your mutable buffer. You write to a socket from your const
buffer. We could explore alternatives, but then there would be a lack
of consistency. While I am usually not one to prioritize consistency
over every other consideration, in this particular case it does make
sense.

Regards

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [JSON][review] Some early feedback

Boost - Dev mailing list
Am 10.09.20 um 17:44 schrieb Vinnie Falco:

> On Thu, Sep 10, 2020 at 8:15 AM Alexander Grund via Boost
> <[hidden email]> wrote:
>>> s.read( buf ) writes data to the buffer you supply
>> I get the rationale. But reading that code I would expect that "s" reads
>> from "buf". So to me the behavior is confusing and hence error prone.
>> Could you explore alternatives?
> I agree that there is some confusion, but I believe that confusion is
> unavoidable. In other words no matter what the verb, it will always be
> subject to interpretation both ways. The way I "break the tie" is
> simply to follow the convention used in networking. You read from a
> socket into your mutable buffer. You write to a socket from your const
> buffer. We could explore alternatives, but then there would be a lack
> of consistency. While I am usually not one to prioritize consistency
> over every other consideration, in this particular case it does make
> sense.
How about: `s.read_into(buf)`? That almost reads like English and makes
it clear(er). This can also be misinterpreted but may be slightly better



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [JSON][review] Some early feedback

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Alexander Grund wrote:
> > s.read( buf ) writes data to the buffer you supply
>
> I get the rationale. But reading that code I would expect that "s" reads
> from "buf".

¯\_(ツ)_/¯

I'd certainly not expect any such thing if s were File or an istream.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [JSON][review] Some early feedback

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 9/10/2020 10:00 AM, Niall Douglas via Boost wrote:

> 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.

Did I miss something or did std::optional ( or boost::optional ) go away
as a means of saying that some value does not exist ? The nullptr is for
a null pointer. Using it otherwise seems wrong, even if the value is the
same size of a nullptr.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [JSON][review] Some early feedback

Boost - Dev mailing list
On Thu, Sep 10, 2020 at 10:01 AM Edward Diener via Boost
<[hidden email]> wrote:
> Did I miss something or did std::optional ( or boost::optional ) go away
> as a means of saying that some value does not exist ? The nullptr is for
> a null pointer. Using it otherwise seems wrong, even if the value is the
> same size of a nullptr.

"null" in JSON does not mean "value does not exist." It means... well,
it is a null. Some people use the word monostate, which I find
pretentious. optional, std or otherwise, has nothing to do with this.
A JSON variant container needs a way to have a null assigned to it,
just like all the other types (integers, floating point). Example:

    json::value jv;
    jv = 1;          // assign an integer
    assert( jv.is_int64() );

    jv = 3.14;     // assign a double
    assert( jv.kind() == json::kind::double_ );

    jv = nullptr;  // assign a null
    assert( jv.is_null() );

I could have gone with this syntax:

    jv = json::null;

by providing my own type and constant:

    namespace json {
    struct null_t {};
    inline static null_t null;
    }

but nullptr is quite close to it and entirely accessible by the common
folk that this library targets, and this avoids an unnecessary
proliferation of small types.

Regards

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [JSON][review] Some early feedback

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On do, 10. sep 18:54, Peter Dimov via Boost wrote:
> Alexander Grund wrote:
> > > s.read( buf ) writes data to the buffer you supply
> >
> > I get the rationale. But reading that code I would expect that "s" reads
> > from "buf".
>
> ¯\_(ツ)_/¯
>
> I'd certainly not expect any such thing if s were File or an istream.
+100

In the networking TS and Asio traditions, I expect all buffer arguments to mean "using this buffer". So, it means "read, using this buffer".

Seth

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost