[JSON][review] Some early feedback
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;
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 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.
On Thu, Sep 10, 2020 at 10:01 AM Edward Diener via Boost <boost@lists.boost.org> 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
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.
On Thu, Sep 10, 2020 at 6:34 AM Andrzej Krzemienski <akrzemi1@gmail.com> 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
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?
On Thu, Sep 10, 2020 at 8:15 AM Alexander Grund via Boost <boost@lists.boost.org> 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
Am 10.09.20 um 17:44 schrieb Vinnie Falco:
On Thu, Sep 10, 2020 at 8:15 AM Alexander Grund via Boost <boost@lists.boost.org> 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
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
participants (7)
-
Alexander Grund -
Andrzej Krzemienski -
Edward Diener -
Niall Douglas -
Peter Dimov -
sehe -
Vinnie Falco