|
|
Boost : |
From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2020-09-21 18:20:26
On Mon, Sep 21, 2020 at 11:04 AM Harshada Wighe via Boost
<boost_at_[hidden]> wrote:
> 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:
> 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.using_boost>
"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
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk