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: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2017-07-01 23:42:49

On Sat, Jul 1, 2017 at 2:39 AM, Michael Caisse via Boost <
boost_at_[hidden]> wrote:

> The formal review of Vinnie Falco’s Beast library will take place from
> July 1 through July 10, 2017.
>
> Please consider participating in this review. The success of Boost is
> partially a result of the quality review process which is conducted by
> the community. You are part of the Boost community. I will be grateful
> to receive a review based on whatever level of effort or time you can
> devote.
>

Aright, you got me.

[snip]

One thing up top: igzf what you call your library.  You're the author, you
get to name it.  We have Spirit (w/Qi and Karma), Hana, Phoenix, and Proto
already.  No one can claim that convention around here is that a library's
name matches its uses.

Some other questions you might want to consider answering:
>
>   - What is your evaluation of the design?
>

* Overall, I quite like it!  I have some complaints, but they are about
small design choices.  The largest design choices, of adhering closely to
the now-begin-standardized ASIO/Net-TS, and to stay in a layer just above
ASIO (and not much higher), are the right ones.  Also, making the library
explicitly standards-tracked is a good choice.  We need more of that in
Boost these days.

* Allocators are gross.  I know this is a controversial position, but I
think they're far more trouble than they're worth.  Since you already have
buffers that seem to me to cover all the major allocation strategies
already, can't you get away with adding a buffer adapter that takes any
random access container as its underlying storage, and be done with it?
This is an effort to reduce your maintenance workload, and the semantic
overhead of users.  Introducing allocators and then trying to use them
uniformly has lead to some awful stuff in the standard, like allocators in
the std::pair and std::tuple ctors, even though they do not allocate.  Yuck.

* Is there a reason why buffer_cat_view and buffer_prefix_view are distinct
types?  It isn't obvious just from their descriptions.  Moreover, fewer
user-facing types is better.  I would greatly prefer to write code like
this:

buffers_view<BufT0, BufT1, ...> buffers = buffer_cat(buf0, buf1, ...);
buffers_view<BufT0, BufT1, ...> buffers_slice = buffer_slice(0, prefix_end);
buffers = buffers_slice(offset, size);

To accomplish this, I think you just need to make a single type that owns a
sequence of buffers, and maintains a pair of iterators or indices into the
owned sequence.  If there is a deeper reason that I have not noticed for
why the current design should remain as-is, please add this to a Rationale
section.

* Similarly, as a user I'd prefer not to have string_body as a distinct
type (unless it's a convenience typedef for dynamic_body<std::string>).  If
std::string is not a model of DynamicBuffer, I think DynamicBuffer could
stand a slight reformulation.

* I get why message<> has a Body template parameter, but why is Fields
configurable?  There is no message<> instantiation under examples/ or
include/ that uses anything other than basic_fields<> that I could find.
Do people ever use something else?  Again, a brief rationale would be
useful.  I saw the "HTTP Message Container" page, btw, but while it
suggested someone might want a different Fields template param, it didn't
indicate why.

  - What is your evaluation of the implementation?
>

I did not look much at the implementation, except as quick checks against
the documentation.

>   - What is your evaluation of the documentation?
>

Very good.  It has lot and lots of concrete examples of how to use
different parts of the library in many different ways.  That's really
important for a large library.

* The Quick Start contains its subsections in full, and then there are TOC
links to each subsection separately.  Probably one or the other would do.

* Each of the example links takes me to a GitHub page for the file linked.
That should change to inline source like the Quick Start code.  The same
thing is true of all the reference documentation.

* HTTP Crawl seems to be doing synchronous reads.  Is there an example
elsewhere that does a bunch of reads like this asynchronously?

* example/http-server-fast/http_server_fast.cpp explicitly checks for '/'
path separators, and uses strings instead of filesystem::paths.  It would
be cool if the former were changed for Windows folks, and the latter
changed, just 'cuz.

* At least http_server_small.cpp (and maybe others? I didn't go back and
check) has only Christopher M. Kohlhoff as an author.  Vinnie, you probably
need a copyright audit of all your files.

* I found the mix of function and type entries in "Table 6. Buffer
Algorithms" to be initially quite confusing.  Maybe separate them?

* The buffer_prefix_view documentation in "Table 6. Buffer Algorithms"
 says "This is the type of buffer returned by buffer_prefix.", but 2/3
overloads with that name return something else.

* HTTP Message Container says this:

"
Now we can declare a function which takes any message as a parameter:

template<bool isRequest, class Fields, class Body>
void f(message<isRequest, Fields, Body>& msg);
This function can manipulate the fields common to requests and responses.
If it needs to access the other fields, it can do so using tag dispatch
with an object of type std::integral_constant<bool, isRequest>.
"

This strikes me as odd advice.  With pre-C++17 I would write:

template<class Fields, class Body>
void f(message<true, Fields, Body>& msg)
{ /*...*/ }

template<class Fields, class Body>
void f(message<false, Fields, Body>& msg)
{ /*...*/ }

Why would I have a third overload and use tag dispatching?  Most users are
going to be really confused by what I quoted above, I think.

Of course, with C++17 and later I'd write:

template<bool isRequest, class Fields, class Body>
void f(message<isRequest, Fields, Body>& msg)
{
    if constexpr (isRequest) {
        // ...
    } else {
        // ...
    }
}

.. and that lines up nicely with the given rationale (just not the tag
dispatching bit).

* Please add an explicit Rationale section for smaller design choices.  The
FAQ and design justification sections that exist are fine, but they mostly
cover the biggest-picture items. Each of those small design choices are
going to go out the window if you can't remember a strong argument for one
of them 5 years from now, when LEWG are asking you why a certain interface
is the way it is.

  - What is your evaluation of the potential usefulness of the library?
>

High.  Super duper high.  The fact that I have no standard way to do
anything in this library today, and this library is
standardization-oriented is a great thing.

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

I did not use the library.

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

I spent about 3 hours reading documentation and examples, with a quick
glance at some of the implementation.

>   - Are you knowledgeable about the problem domain?

Somewhat.  I've written a couple of Node.js-based web servers, a couple of
straight-ASIO non-web servers and clients, and done a bit of web
development.  I've done nothing that works at exactly the level Beast
targets.

Zach