[JSON[review] Andrzej's review

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

[JSON[review] Andrzej's review

Boost - Dev mailing list
Hi Everyone,
I recommend to *ACCEPT* the library into Boost. It meets the standard for
BI am definitely going to use it.

I tried to use the library in two configurations in header-only mode:
g++ 9.2.0 with -std=c++2a on MinGW on Windows,
g++ 10.2.1 on Fedora with -DBOOST_JSON_STANDALONE and -std=c++20
All worked fine.

I studied library docs and played with examples for about 30 hours. I am
not particularly knowledgeable about the problem domain. But my programs
have to consume and produce JSON files.

DOCUMENTATION

It is very good, and meets the highest Boost standards. I mean both the
tutorial and reference sections. While doing this review I had to look at
the documentation of the competing libraries, and they are really bad. This
is why I can appreciate the big effort you put in making these docs. There
is a section "Comparison" that compares Boost.JSON to other JSON libraries.
I personally think it should also mention the documentation.

Some observations:

1. Add reference to RFC 7159 (https://www.rfc-editor.org/info/rfc7159). It
allows implementations to set limits on the range and precision of numbers
accepted.

2. The documentation for array object and index mention "Satisfies the
requirements of ContiguousContainer, ReversibleContainer, and
SequenceContainer" These requirements are never defined or referenced. The
user will not even know if you defined them or if they are "commonly
known". Maybe add link to cppreference.com:

https://en.cppreference.com/w/cpp/named_req/ReversibleContainer
https://en.cppreference.com/w/cpp/named_req/ContiguousContainer
https://en.cppreference.com/w/cpp/named_req/SequenceContainer

3. String has two sections "Member Functions":
https://master.json.cpp.al/json/ref/boost__json__string.html

4. Documentation for value::operator= seems corrupt:
https://master.json.cpp.al/json/ref/boost__json__value/operator_eq_/overload4.html
* it has sections with no content
* It is an unconstrained template, it look like it can accept any type

5. Examples of input JSON-s contain a lot of escape characters in string
literals, like in
https://master.json.cpp.al/json/usage/conversion.html
This makes it difficult to read. Consider using raw string literals.
Instead of
assert( serialize( jv ) == "{\"x\":4,\"y\":1,\"z\":4}" );
We would have
assert( serialize( jv ) == R"({"x":4,"y":1,"z":4})" );

6. Some functions (like operator== or swap(x, y)) are naturally part of
class interface, even though they are not member functions. It is annoying
and confusing that rather than being listed along with the class they
belong to they are instead listed separately as "free functions"

The docs are not uniform about this issue. For `json::value` operator== is
listed as friends along with other member functions, but not 2-argument
swap().

7. In  https://master.json.cpp.al/json/usage/conversion.html we read:
" Given the following definition of customer::customer( const value& )
[...] Objects of type customer can be converted *to* and from value".

This "to" is surprising here. How can a constructor converting from `value`
to `customer` could be used to convert `customer` to `value`?

8. As discussed in another thread, documentation would benefit from the
info on implementation-limits-related design choices.

a. Any json::value that you can build can be serialized and then
deserialized, and you are guaranteed that the resulting json::value will be
equal to the original.

b. JSON inputs where number values cannot be represented losslessly in
uint64_t, int64_t and double, may render different values when parsed and
then serialized back, and for extremely big number values can even fail to
parse.

c. Whatever JSON output you can produce with this library, we guarantee it
can be passed by any common JSON implementation (probably also based on
uint64_t+int64_t+double implementation.


DESIGN

Clear, sound and practical. The design decisions correspond with my
expectations of a library: fast, easy to use and learn, with value
semantics. The interface is very intuitive, I practically do not have to
consult the documentation: every functionality (serialization, value
alteration) has the interface that I would expect. I appreciate that it
targets embedded environments.

Some observations:

1. The handling of infinities and NaN's needs to be addressed. Currently, I
can do:
`json::serialize(std::numeric_limits<double>::infinity())`
and t produces an invalid JSON. You cannot easily make a precondition on
json::value assignment/construction because you expose the double as a
mutable member:

void fun(double x)
  // precondition x is not infinity or nan
{
  json::value v = x; // ok
  v.as_double() *= 2.0; // this may produce infinity
}

So maybe you need a precondition in the serializer.

2. Boost.JSON provides its own string type. This begs a lot of design
questions for `json::string`. Is it a better replacement for std::string?
std::string provides the following capability:

```
void set(std::string& s, char v, char x, char z, char k)
{
   if (cond)
     s = {v, k};
   else
     s = {v, x, z, k};
}
```

json::string does not. How do I do a similar thing with json::string?

3. string::swap() has a strange precondition: you cannot swap an object
with itself
https://master.json.cpp.al/json/ref/boost__json__string/swap.html

This is a really unexpected and potentially dangerous, and at minimum
requires a very strong rationale. This would be the first swap function in
std or Boost that has a precondition. While discussing a somewhat related
issue (on the guarantees of the moved-from state) in the LEWG, someone
remarked that there was at least one STL implementation that did a
self-swap for convenience of implementation.

Is it really necessary?


IMPLEMENTATION

I didn't look at it much. Just some quick points I observed.

1. Values of type `double` representing Infinity and NaN can be stored in
json::value and when later serialized, they render a malformed JSON
output.  json::value should either validate special values of `double` or
indicate as precondition that they should not be assigned. Alternatively,
the serializer should either validate this or indicate as precondition.

2. tag_invoke() takes tag arguments by reference, does it have to? Passing
by reference requires the constexpr variables to be instantiated and their
address assigned.

3. Customization through ADL-discovered overloads can cause ODR violations.
There is no superior alternative though. But maybe put a warning in the
docs that the user must make sure that all translation units must see the
same set of overloads.

4. I do not like the 2-state-switch mechanics of BOOST_JSON_STANDALONE. It
assumes that a user either wants a no-Boost version or that she wants a
package: all Boost components.

But what if I am using C++14 and I want to use std::error_code and
Boost-emulated memoty_resource and string_view? What If I use Boost version
and C++17 and want to use std::string_view?

Maybe you can additionally provide macros for turning Boost-emulation
libraries one-by-one? I mean, one for string_view, one for error_code, and
one for memory_resource.

5. The header only mode -- I really appreciate it. It is good for initial
experiments.


Finally, I would like to thank Vinnie and Krystian for writing and sharing
this library.

Regards,
&rzej;

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

Re: [JSON[review] Andrzej's review

Boost - Dev mailing list
Andrzej Krzemienski wrote:

> 2. tag_invoke() takes tag arguments by reference, does it have to?

In principle, pass by reference is idiomatic for tag_invoke tags because
this allows tag_invoke overloads to be defined without including any header
from the corresponding library. You can forward declare the tags.

This does not apply here though, as you need to have the declaration of
json::value visible in order to define the tag_invoke overload. So it's
possible to pass the tags by value and nothing would be lost.


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

Re: [JSON[review] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Thu, Sep 17, 2020 at 11:10 AM Andrzej Krzemienski via Boost
<[hidden email]> wrote:
> I recommend to *ACCEPT* the library into Boost.

Thank you for your time and thoughtful comments.

> 3. String has two sections "Member Functions":
> https://master.json.cpp.al/json/ref/boost__json__string.html

That's a bug in the doc toolchain, the 2nd section is "Static Member
Functions" (max_size is static)

> 4. Documentation for value::operator= seems corrupt:
> https://master.json.cpp.al/json/ref/boost__json__value/operator_eq_/overload4.html
> * it has sections with no content

Just a case of missing text, fixed here:

<https://github.com/CPPAlliance/json/blob/75794a7f372fb36225946c989d2ca6cdb6d94f01/include/boost/json/value.hpp#L1056>

> 5. Examples of input JSON-s contain a lot of escape characters in string
> literals, like in https://master.json.cpp.al/json/usage/conversion.html

I'm not happy with this section at all, it needs a rewrite.

> 6. Some functions (like operator== or swap(x, y)) are naturally part of
> class interface, even though they are not member functions. It is annoying
> and confusing that rather than being listed along with the class they
> belong to they are instead listed separately as "free functions"

Yes I know what you mean, and this is a limitation of the doc scripts
I am using. `operator==` is an inline friend to accelerate overload
resolution, but `swap` is not. I can make `swap` an inline friend as
well, if that doesn't cause any problems (with respect to the rules of
C++).

> 2. Boost.JSON provides its own string type. This begs a lot of design
> questions for `json::string`. Is it a better replacement for std::string?
> std::string provides the following capability:
> ...
> json::string does not. How do I do a similar thing with json::string?

json::string is a better replacement for std::string for use as a type
in the variant, because it is optimized to work with the library. As
for the initializer_list<char> overloads, well - I have my doubts that
these are used outside of toy examples, but if a user complains then I
guess we would add them.

> 3. string::swap() has a strange precondition: you cannot swap an object
> with itself
> https://master.json.cpp.al/json/ref/boost__json__string/swap.html

Just open an issue and explain how you think self-swap, self-move, and
self-assignment should work and I will make the changes.

> 4. I do not like the 2-state-switch mechanics of BOOST_JSON_STANDALONE. It
> assumes that a user either wants a no-Boost version or that she wants a
> package: all Boost components.
>
> But what if I am using C++14 and I want to use std::error_code and
> Boost-emulated memoty_resource and string_view? What If I use Boost version
> and C++17 and want to use std::string_view?
>
> Maybe you can additionally provide macros for turning Boost-emulation
> libraries one-by-one? I mean, one for string_view, one for error_code, and
> one for memory_resource.

Well, I don't like having two versions of error_code, two versions of
string_view, two versions of memory_resource, two versions of Asio,
and so on. If 90% of the C++ community was using C++17 or later,
Boost.JSON might *only* have the standalone mode. In other words, I
would probably not support boost::system::error_code and other Boost
types. This of course is assuming that Asio was using the std
equivalents rather than the Boost versions which it currently uses.

Practically speaking, C++11 and C++14 have enough users that
supporting them makes sense. Thus what I have done is create two
modes. The "standalone" mode, which users must opt-in to, and the
Boost mode which is what you get by default. Note that these are two
distinct libraries with their own set of symbols. Both versions can
co-exist in the same executable, without getting link errors. This is
accomplished through inline namespacing.

At some point, Boost is going to have to holistically address this
issue of having its own substitute types for commonly used std types.
But for now I think it is best that users make a choice to either go
all-in on Boost types, or simply go with C++17 and have all-std types.
I would like to avoid enabling users to proliferate more code that
mixes and matches the Boost types with the std types.

Thanks

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

Re: [JSON[review] Andrzej's review

Boost - Dev mailing list
Vinnie Falco wrote:

> Just open an issue and explain how you think self-swap, self-move, and
> self-assignment should work and I will make the changes.

They should be no-ops.


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

Re: [JSON[review] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> Vinnie Falco wrote:
>
> > Just open an issue and explain how you think self-swap, self-move, and
> > self-assignment should work and I will make the changes.
>
> They should be no-ops.

More specifically, self-swap should be a no-op, and assignments should be
equivalent to:

T& operator=( T const& rhs )
{
    T(rhs).swap(*this); return *this;
}

T& operator=( T&& rhs )
{
    T(std::move(rhs)).swap(*this); return *this;
}

This is important for classes such as json::value, object, array, where
*this may own rhs, so it's important not to destroy rhs too early. E.g.

    v = v.at(0);
    v = std::move(v.at(0));

    a = a.at(0).as_array();
    a = std::move(a.at(0)).as_array();

    o = o.at("key").as_object();
    o = std::move(o.at("key").as_object());

This is not self-assignment, but it has the same lifetime pitfalls - if you
destroy the old contents of *this, rhs is invalidated, so you can no longer
copy (or move) it.


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

Re: [JSON[review] Andrzej's review

Boost - Dev mailing list
On Thu, Sep 17, 2020 at 2:03 PM Peter Dimov via Boost
<[hidden email]> wrote:
> T& operator=( T const& rhs )
> {
>     T(rhs).swap(*this); return *this;
> }

The memory_resource associated with a value can never change after
construction, so the above would have to be written as:

    T& operator=( T const& rhs )
    {
        T( rhs, this->storage() ).swap( *this );
        return *this;
    }

and

    T& operator=( T const& rhs )
    {
        T( std::move(rhs), this->storage() ).swap( *this );
        return *this;
    }

Which I think is ok...

Thanks

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

Re: [JSON[review] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Thu, Sep 17, 2020 at 11:10 AM Andrzej Krzemienski via Boost
<[hidden email]> wrote:
> 2. The documentation for array object and index mention "Satisfies the
> requirements of ContiguousContainer, ReversibleContainer, and
> SequenceContainer" These requirements are never defined or referenced. The
> user will not even know if you defined them or if they are "commonly
> known". Maybe add link to cppreference.com:

Fixed in develop, thanks.

> 4. Documentation for value::operator= seems corrupt:
> https://master.json.cpp.al/json/ref/boost__json__value/operator_eq_/overload4.html
> * it has sections with no content
> * It is an unconstrained template, it look like it can accept any type

Fixed in develop.

> 6. Some functions (like operator== or swap(x, y)) are naturally part of
> class interface, even though they are not member functions. It is annoying
> and confusing that rather than being listed along with the class they
> belong to they are instead listed separately as "free functions"

Fixed in develop.

> The docs are not uniform about this issue. For `json::value` operator== is
> listed as friends along with other member functions, but not 2-argument
> swap().

Fixed in develop.

Thanks

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