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

Subject: Re: [boost] [Fit] formal review ends 20th March.
From: Paul Fultz II (pfultz2_at_[hidden])
Date: 2016-03-21 13:55:57

On Sunday, March 20, 2016 at 8:21:09 PM UTC-5, rstewart wrote:
>
> "Vicente J. Botet Escriba" <vicent..._at_[hidden] <javascript:>> 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. 
>

The library does require C++11 as mentioned in the first sentence. However, 
this is somewhat ambiguous, so its best too look at what compiler are 
supported. 

The Fit library provides simple function utilities that can provide a 
simpler alternative to many template metaprogramming constructs. It doesn't 
require expertise in TMP, and the point of the "relevant" section is it 
doesn't require expertise in functional programming as well. Although 
background in both of these areas can be helpful.
 

>
> 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. 
>

A TOC is provided for all headers on all pages.
 

>
> Function Objects 
>
> You state that the function call operator is always declared const.  On 
> what basis do you make that claim?  

This I plan to discuss more. There are two main reason for this:

1) Mutable function objects are problematic with many surprises. Most of 
the time `std::ref` should be used instead.

2) With C++ lambdas, mutability is explicit, and the Fit library follows 
this convention as well.
 

> 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. 
>

I just wanted to make clear that mutability is not completely prohibited.
 

>
> 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. 
>

Yes, I think that is better phrased. Although, I plan to add a deeper 
discussion about how functions are declared.
 

>
> 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.  

The point is to show both forms are available, and the later examples use 
the functional forms. Perhaps it would be better to not discuss the 
"_adaptor" form in this section at all.
 

> 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. 
>

I write, "Instead of writing function objects which can be a little 
verbose, we can write the functions as lambdas instead.". That is why they 
are useful. How should I word it to make it more clearer?
 

>
> 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. 
>

The example I give for match doesn't have much advantage. It is only used 
to transition to 'conditional'. However, match is useful for when you are 
using visitors.
 

>
> 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.

I'll move through this more slowly.
 

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

Hmm. The first three sentences explain what the 'for_each_tuple' code does. 
I don't understand the disconnect here. Although, I could add an example of 
using 'for_each_tuple', which might help.
 

>
> 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.) 
>

Your reasoning about it is wrong, and the code is wrong, as pointed out by 
this issue here:

https://github.com/pfultz2/Fit/issues/131

Either way, I plan to drop this part, as it just complicates the quick 
start guide.
 

>
> 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.) 
>

Some examples would be good.
 

>
> 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." 
>

I was planning to rename this page to "Definitions".
 

>
> 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. 
>

Why? What part is not clear?
 

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

Why? What part is not clear? 

> 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. 
>

This is not describing functionality, but just notation used in the library.
 

>
> 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. 
>

It is a feature. Since functions are designed this way, it makes it easier 
to pass them to other functions(such as writing `compose(unpack, by)`).
 

>
> 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. 
>

There has been a lot of feedback about the documentation and I plan to 
rewrite a lot of the documentation.
 

>
> > - 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. 
>

Thanks Rob for the review.
 

>
> ___ 
> Rob 
>
> _______________________________________________ 
> Unsubscribe & other changes: 
> http://lists.boost.org/mailman/listinfo.cgi/boost 
>