On Mon, Sep 21, 2020 at 11:04 AM Harshada Wighe via Boost
We vote to reject the JSON library from Boost and our review of the library and reason for rejection are below:
Thank you for spending the time to review this library.
1. Some files do not include all headers at the very start of the file but have some include statements at the end.
Yes this is explained in the comment: https://github.com/CPPAlliance/json/blob/6ddddfb16f9b54407d153abdda11a29f746...
2. Splitting the files into HPP and IPP is also not good because it spreads the class into more files than necessary and causes compilation to be slower.
Did you measure? .ipp files are compiled into the library. This arrangement actually causes compilation to be faster, as the library's function definitions are only compiled once and not included in every one of the user's translation units.
3. The library is complicated to use in a project because you cannot simply include headers in any file you want. You have to include a src.hpp file but this file must be included only once.
No that's not correct. Boost.JSON can be used just like every other Boost library. You include the public headers you want, and then link with the compiled library. This is explained in the documentation: https://master.json.cpp.al/json/overview.html#json.overview.requirements.usi... "src.hpp" provides an alternative to linking with a compiled library, it is another way to use Boost.JSON.
5. Compared to our in house JSON library, this library was slightly slower.
Sample code please.
The parser allocates too much memory:
6. I think this is because the design is not optimized for the case when all the JSON content is already stored in memory. In our parser we point to the strings in the original buffer.
Instances of json::value produced by the parser are first-class, Regular data types; they are copyable, movable, assignable, etc... "in-situ parsing" (what you refer to as pointing to strings in the original buffer) is a specialized use-case which does not produce Regular data types. In other words it is not a focus of Boost.JSON.
7. In json/config.hpp there are implementations of move and remove_cv from the C++ standard. This introduces more duplicate of this functionality into programs.
This is removed in the develop branch, it was an early experiment to reduce the amount of header material brought in from the standard library. Boost.JSON design choices emphasize speed of compilation.
The allocator design is not standard:
std::pmr::memory_resource is standard as of C++17. For earlier versions of C++, boost::container::pmr::memory_resource is used.
We did not understand why storage_ptr was needed at first.
Explained in the documentation: https://master.json.cpp.al/json/usage/allocators/background.html
It would be better for the library to just template on allocator just like the vector class in the C++ standard.
I disagree. Templates are completely unnecessary for memory allocation, as this library demonstrates. Making json::value be a template would put all of the function definitions into the public headers, and increase the size of the executable when different allocators are instantiated in the same program. It would also hinder interoperability: every library that wants to use JSON types in its public interfaces would then be required to use function templates, or to pick an arbitrary Allocator type. Neither of these are desirable outcomes.
The allocator system also uses virtual calls. The C++ standard vector class also does not have this problem when it uses the standard allocator system.
std::pmr::vector, which uses std::pmr::polymorphic_allocator is standard as of C++17. For earlier versions of C++, boost::container::pmr::vector is available which uses boost::container::pmr::polymorphic_allocator. You have not stated why virtual calls are a problem. Did you measure? Regards