Re: [boost] [Fit] formal review ends 20th March.

Subject: Re: [boost] [Fit] formal review ends 20th March.
From: rstewart (rstewart_at_[hidden])
Date: 2016-03-20 21:20:39

"Vicente J. Botet Escriba" <vicente.botet_at_[hidden]> wrote:
>
> We encourage your participation in this review. At a minimum, kindly 
> state:
> - Whether you believe the library should be accepted into Boost
>   * Conditions for acceptance
> - Your name

Rob Stewart

> - Your knowledge of the problem domain.

I'm neither expert nor overly experienced due to the need to support LCD compilers, but reasonably well educated on the topic.

> You are strongly encouraged to also provide additional information:
> - What is your evaluation of the library's:
>   * Design
>   * Implementation
>   * Documentation
>   * Tests
>   * Usefulness

Without meaning to beat a dead horse, I must say that the initial documentation page is off-putting.  The TOC is very long, and there's very little to help me understand the point.  For example, the Introduction tells me that the library, "provides utilities for functions and function objects."  My first question is, why do I need utilities for such things?  I've been writing them for years without difficulty.  The page descibes language facilities it uses, but I don't know why nor do I care without knowing how the library will actually benefit me.  The first page of the documentation has to sell me on the library.  It should also clearly identify the audience.  If a potential user is inexperienced with TMP, will the library hurt or help?  If the user is a heavy user of lambdas how will the library help?  If the user can't use C++11, will the library help?  If you make me work this hard to discover whether I can get value from your library, I'm going to ignore it.

Once you've identified your audience, ensure that the terminology and examples are targeted to that audience.  If you think your library should appeal only to expert functional or TMP programmers, say so and then speak to them in language they will recognize.  If your library should appeal to folks that don't understand much about functional programming, but will lead them along that path, be sure to describe things in terms non-functional programmers will understand.  If your library should appeal to a broad range of users, then you may well need more than one kind of introduction or tutorial.  That is, you might give those with more experience or background a quick walk through the range of features and lead them quickly to more advanced examples and the reference material, while you give the rest a gentler introduction.

Without that information, I really don't know how useful the library is to me and the design, implementation, and tests are largely irrelevant.  That said, I will continue to give more impressions based upon reading through more of the documentation.

Quick Start

This page is broken into sections with headings and provides a TOC, yet the sections are not independent.  Instead, the content must be read from top to bottom to make sense.  Therefore, the TOC is misleading.

Function Objects

You state that the function call operator is always declared const.  On what basis do you make that claim?  It may be a reasonable, even common thing to do, but you should justify your assertion.  That such is needed for working with Fit is not even relevant at this point in the documentation.  You were just describing a function object.  Mentioning mutable_ at this point is most definitely too much information.

You write, "construct the class as a global variable," which is phrased incorrectly.  That should be something like, "construct an instance of the class in a global variable."  What's more, you should rephrase that sentence to highlight Fit's role better: "Fit provides an easy means to put an instance of the class in a global variable, with no ODR problems, that can be invoked like an ordinary function."  That would explain the point a little better and specifically address the ODR concerns raised during the review.

Adaptors

s/Now we have/Now that we have/

You note that the function could be made pipable without explaining what that means or why it's valuable.  This sort of thing works better if you provide a use case and tentative syntax and then show how Fit can make that a reality.

You go on to discuss the variants without the "_adaptor" suffix without any explanation of why that is of any use.  You assume that the reader can infer, from the terse examples offered, their own uses.  At the very least, some description of, or reference to, partial application of functions would be of help here.  Still, the point is that you presume much more than I think you should of the reader.

Lambdas

This section looks ugly, because of all of the macros you've used, and makes no attempt whatsoever to explain why any of the things shown would be useful.

Overloading

This section assumes that the reader will make inferences that initializing lambdas at compile time and doing something special with overloading that we apparently don't get from the language itself, Fit improves the programmer's life.  Guess what?  That assumption is faulty.

I can see that conditional might be preferable to match because it doesn't apply overload resolution and, therefore, doesn't lead to the same ambiguities.  However, the question remains why I would want either in the first place.

The last sentence, refers to the example that follows it, but is worded as if it refers to the previous example as the sentence before it did.  To make things worse, the helper function mentioned is not named, though it can be inferred from reading the following code, and it is not explained.

Tuples

There's an awful lot packed into the first three sentences and nothing in them actually explains what the following code does.

I really don't know what, "Since we can't use a lambda inside of decltype we just put identity instead," is supposed to mean.  I can see that you reference identity in the example code, but I really don't know why.  (I can reason that somehow identity will be the tuple type, but that's a lot to swallow with no explanation.)

Recursive

I can guess that the point of this section is to make the print function object print objects within objects using recursive invocations of print, but you need to describe that and even show sample output for some inputs.  (That illustration of outputs for inputs idea should be applied to each section.)

Variadic

This section makes the most sense of them all.  You actually explained why making the function variadic would be useful and the modification was explained and then shown.

After having worked to get this far through the documentation, I would have walked away were I not writing this review.  You've thrown mutable_, pipable_adaptor, match, conditional, by, unpack, identity, fix, and adl::adl_begin() at me with very poor explanations to develop an example that I can only guess might have some narrow application to my code.  Frankly, I would have walked away before I reached the end.

More examples

"a key point of many of these utilities is that they can solve these problems with much simpler constructs than whats traditionally been done with metaprogramming"

That's it!  Why wasn't that on the first page!  That's the thesis of this library.  That's what you need to focus on as you write the documentation.  Show how Fit makes things simpler.  Highlight my pain and show me how to reduce or eliminate it.

What's more, this page is exactly what I expected to be the Motivation page.  You've described real world problems and shown how Fit can solve or simplify them.

In the Projections section, you wrote, "Instead of writing the projection multiple times in algorithms."  I can infer that you mean one had to write ".year_of_birth" multiple times, but I'm not familiar with the term "projection" in that context.  Be careful what you assume your readers know.

Near the bottom, you refer to "UFCS" without defining the acronym or linking to a definition.

s/template classes/class templates/
s/bit or/bitwise OR/

Overview

Why is the third page in the documentation called "Overview"?  That's normally what one expects right at the beginning.  In reality, I think you mean to call this page something like, "Library Features."

There really should be an introductory sentence explaining the point of this page, which I infer to be a summary of the key features of the library.

Static Function Adaptor

I have no idea what, "A static function adaptor is a function adaptor that doesn't have a functional form," is supposed to mean.

Decorator

I have no idea what, "A decorator is a function that returns a function adaptor," is supposed to mean.

Semantics

This section is in the middle of sections describing main areas of functionality in the library, so it's out of place.  This is especially true given the links in the page TOC.  This content should follow the TOC with no heading as part of a general introduction to the page.

Signatures

This section also seems to be a general statement about content to be found elsewhere in the documentation.  It isn't a feature of the library, so it confuses me based upon what I was guessing this page was about.

That's as far as I got in the documentation.  I'm really confused as to the real purpose and scope of this library and on that basis I don't think it should be accepted in its present form.  Once the purpose and scope are well defined, it will be possible to judge how well it fits and whether that should be part of Boost.

> - Did you attempt to use the library?

No

> - How much effort did you put into your evaluation of the review?

Several hours, including following the review discussions.

___
Rob