Re: [boost] [review][beast] Review of Beast starts today : July 1 - July 10

Subject: Re: [boost] [review][beast] Review of Beast starts today : July 1 - July 10
From: Glen Fernandes (glen.fernandes_at_[hidden])
Date: 2017-07-05 01:30:46

What follows is my review of the Beast library:

A. Should the library be accepted into Boost?

Yes. I vote ACCEPT, with no conditions for acceptance.

B. What is your evaluation of the design?

Good. As someone who is only barely familiar with using ASIO, it took some
time to understand and appreciate the library. Beast is designed with the same
goals that make the much of C++ standard library and other Boost libraries
useful: It tries to provide as much control to the user of the library, and
aims to make as few assumptions as possible that would alienate potential use
cases. If there is an avenue for customization, whether it is the policy for
dynamic allocation or otherwise, the library tries to expose it to the user.

C. What is your evaluation of the implementation?

The implementation is of a high quality,  implemented using modern idiomatic
C++, careful chooses dependencies on other well maintained Boost libraries
when possible to avoid duplicating already well-implemented functionality. It
has generally correct support for the C++11 allocator model. Many of the
interfaces which are templates are additionally constrained with compile time
checks and assertions, to make sure that any user supplied types satisfy those
concepts correctly.

The library code looks to be well covered by tests, and the library's test
suite appears extensive, inluding benchmarks, code coverage, as well as tests
for implementation detail components (like various traits and helper
functions).

D. What is your evaluation of the documentation?

Excellent. Even though I believe the interfaces are well designed, the library
is sufficiently complex that without its current level of documentation, it
would have taken me several additional hours (especially as non-ASIO user) to
understand how to use it.

E. What is your evaluation of the potential usefulness of the library?

It looks very useful, albeit _to me_ (given my unfamiliarity with HTTP,
and with ASIO), not very simple to use without the aid of the examples.
Intuitively, I can see developers proficient with the domain being able
to use it to write powerful high level libraries or applications.

F. Did you try to use the library? With which compiler(s)?
   Did you have any problems?

Yes. I tried the tests, examples, benchmarks with gcc 6.3 (with libstdc++),
with clang 3.9.1 (with libstdc++ and libc++). Additionally I wrote a HTTP
client program by copying an example and modifying it.

G. How much effort did you put into your evaluation? A glance? A quick reading?
   In-depth study?

Not more than a fwe hours over the course of this review, which includes:
- Getting a little familiar with ASIO.
- Reading through the Beast implementation and examples code.
- Building and running the tests and examples
  (with b2, against current Boost master branch).
- Using Beast to write a simple HTTP client program.

H. Are you knowledgeable about the problem domain?

Not about HTTP, Websockets, or even the ASIO library.
(Only about TCP, sockets, and general network I/O).

Some minor suggestions, after reading through the latest code:

1. Test Jamfile should be changed so that b2 simply works,
   without first having to run b2 headers.

2. Include C library facility headers after C++ standard library headers.
   For example:

    #include <memory>
    #include <cstdint>

3. Various default-initialization new[] could be boost::make_unique_noinit:

    p_ = boost::make_unique_noinit<std::uint8_t[]>(capacity_);

    rd_.buf = boost::make_unique_noinit<std::uint8_t[]>(rd_.buf_size);

4. Alternatively:
   Perhaps those cases of std::unique_ptr could instead support allocators.

5. There are clamp() utilities defined in two places
   (ranges.hpp and clamp.hpp).

   Maybe all of these can exist in one place.

6. Trivial utilities (like clamp, ascii_tolower, etc.) could be constexpr.
   Since you support C++11 they can be written as single return expression.

   template<class T>
   constexpr inline T clamp(T x, T y)
   {
       return x < y ? x : y;
   }

7. Rely on Boost.Config when possible for detecting MSVC,
   in case compilers emulate it.

   Use BOOST_MSVC_FULL_VER instead of _MSC_FULL_VER.

8. Change the style of feature detection or defect macros,
   to not be defined to 1 or 0.

   Instead they should just be defined or not defined,
   consistent with Boost.Config.

   #define BEAST_NO_BOOST_STRING_VIEW

   #if !defined(BEAST_NO_BOOST_STRING_VIEW)

9. string_view 'value' parameters might be more idiomatic (and even optimal),
   instead of const-reference parameters.

   bool iequals(string_view lhs, string_view rhs)

10. empty_base_optimization should check is_final on more than just clang.
    Other C++ implementations also support this trait.

11. Maybe basic_multi_buffer(const Allocator&) be marked explicit.

Note: None of the above may be required, actionable, or even worth the author
considering. They are just thoughts that I noted while I was reading the code.

Thank you, Vinnie, for proposing your efforts for inclusion in Boost.

Glen