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

On Sat, Jul 1, 2017 at 2:39 AM, Michael Caisse via Boost < boost@lists.boost.org> 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 buffers = buffer_cat(buf0, buf1, ...); buffers_view 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_bodystd::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 void f(message& 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. " This strikes me as odd advice. With pre-C++17 I would write: template void f(message& msg) { /*...*/ } template void f(message& 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 void f(message& 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