Re: [boost] [Fit] Review

Subject: Re: [boost] [Fit] Review
From: Paul Fultz II (pfultz2_at_[hidden])
Date: 2016-03-09 19:12:08

On Wednesday, March 9, 2016 at 8:50:01 AM UTC-6, Zach Laine wrote:
>
> - Whether you believe the library should be accepted into Boost 
>
> Yes, conditionally. 
>
>
>   * Conditions for acceptance 
>
> I think the documentation and naming issues I raise below, and those filed 
> on 
> GitHub by me and others should be addressed before acceptance.  Also, the 
> ADL 
> issues that Steven Watanabe alluded to previously need to be fixed.  After 
> that, I think it should be re-submitted. 
>
>
> - Your name 
>
> Zach Laine 
>
>
> - Your knowledge of the problem domain. 
>
> I do generic programming regularly, and have done for years. 
>
>
> You are strongly encouraged to also provide additional information: 
> - What is your evaluation of the library's: 
>   * Design 
>
> I think it's good.  I particularly like the fact that it attempts to keep 
> everything constexpr- and SFINAE-friendly wherever possible. 
>
> I *really* like reveal() and failure().  Too few TMP-heavy libraries pay 
> attention to providing good error messages. 
>
> I have no problem with the constness requirements on Callables used 
> throughout 
> Fit, as was discussed previously.  I even consider this a feature, 
> allowing 
> me 
> to detect non-const Callables, which are almost always an error on my 
> part. 
> This is especially true since std::ref() and mutable_() let me explicitly 
> choose how to handle mutable Callables, in a way that is also explicit at 
> the 
> call site. 
>
> There are a number of adaptors that I find have obscure names: 
>
> by() is a projection; why not call it project()? 
>

I chose `by` because of how it is used linguistically. That is you write:
`sort(v.begin(), v.end(), by(&Employee::Name, _<_))` which I read "sort by
employee name". Also, if I write `by(decay, construct<std::tuple>())`, I 
read
"construct tuple by decay". In haskell, it uses `on` for this adaptor, which
is what I originally called it. `project` is a more straighforward name.
 

>
> compress() is defined in terms of "fold", with a link to the Wikipedia 
> page 
> for same; why not call it foldl()?  Similarly, reverse_compress() should 
> be 
> foldr(). 
>

Not `foldr`. This is because `foldr` is symetrical. For example, `foldl` and
`foldr` should produce the same results:

    foldl(_+_, std::string())("Hello", "-", "world"); // "Hello-world"
    foldr(_+_, std::string())("Hello", "-", "world"); // "Hello-world"

However, compress and reverse_compress work like this:

    compress(_+_, std::string())("Hello", "-", "world"); // "Hello-world"
    reverse_compress(_+_, std::string())("Hello", "-", "world"); // 
"world-Hello"

I was reluctant to call it `fold` as it seems to imply some data structure 
to
fold over, whereas this is simply an adaptor. I used the word compress as 
its
a another name used for fold. However, it seems more people would prefer to
use `fold` and `reverse_fold`.

 

>
> flow() is a super obscure name!  Why is this not reverse_compose() or 
> similar? 
>

I actually got the name from underscore.js. The reason why I chose this name
instead of `reverse_compose` is because when this is used with pipable
functions it seems confusing:

    reverse_compose(
        filter([](int i) { return i < 3; }),
        transfrom([](int i) { return i*i; })
    )(numbers);

With the word 'reverse' there, it almost looks as if it computes the 
pipeline
in reverse order, which it doesn't. So I would prefer a name without reverse
in it.
 

>
> indirect() could more easily be understood if it were called deref() or 
> dereference(). 
>

Well, `indirect` is how it is commonly called in libraries such as
Boost.Range, range-v3, and PStade libraries. So I would like to keep the 
name
consistent with other similar constructs.
 

>
> I don't feel as strongly about partial(), but I think it might be clearer 
> if 
> it were called partial_apply() or curry(). 
>

Hmm, I don't think a name with `apply` in it is good name for an adaptor.
 

>
> conditional() should be called invoke_first(), call_first_of(), or 
> similar. 
>  I 
> find it too easy to confuse with if_(). 
>

I see, I was trying to describe an adaptor where you could put functions 
with
conditions in it. Other people, seem to prefer a name like `linear`, so I
might use that instead.
 

>
>
>   * Implementation 
>
> I only briefly looked at the implementation, and only a few adaptors at 
> that. 
> What I saw did not leave me any concerns in particular. 
>
> However, I *am* concerned by Steven Watanabe's claim that the library is 
> ADL-unsafe.  He did not state exactly why that I could tell, but this 
> needs 
> to 
> be addressed if so. 
>
>
>   * Documentation 
>
> The Quick Start section is good at showing a compelling motivating case 
> for 
> using the library.  The result at the end of the section looks like a very 
> good start on a "Scrap Your Boilerplate" solution for simple, dump-style 
> printing.  It's relatively easy to read and write. 
>
> The documentation is what needs the most work, from what I've seen.  The 
> Quick 
> Start is quite nice, but then there are some things that are 
> underexplained 
> in 
> the subsequent Overview and Basic Concepts sections.  For instance, why 
> are 
> these macros used everywhere and what do they do?  They should be used for 
> the 
> first time after showing what the expanded code looks like.  

I don't think the docs should show the expansion of the macros, that is part
of the implementation and not interface. I could show an "idea" what is is
expanding to, with explanation of what else it is doing beyond the simple
explanation.
 

> This will 
> justify 
> their use for those users that want them, and show those that don't what 
> they 
> can write instead. 
>

I could show an alternative without the macros, but I would prefer to put 
that
in the Advance section. In the introduction of the library, I would rather
show the constructs that can be easily used without a need to explain a 
bunch
of caveats.
 

>
> Others have written during the review that they don't understand why this 
> library is useful.  I think this is simply a documentation failure.  I 
> find 
> the printing example from the Quick Start compelling, but perhaps Paul 
> could 
> show how it would have to be done if hand-written (even if only 
> partially). 
> Also, Paul has told me offline that there are a small number of primitive 
> operations on which everything else is built (pack, unpack, and apply, 
> IIRC). 
> Perhaps show how those can be used to implement the other adapters, and 
> how 
> hard they are to write yourself.  Showing how at least some of the library 
> can 
> be used as fundamental building blocks would go a long way. 
>
> Some specifics.  I have read the online docs before, and I put several 
> bugs 
> into the GitHub page for what I consider insufficient examples or 
> explanations.  Some new points, though: 
>
> I think alias() is in bad need of a motivating example. 
>
> I don't understand the distinction between lazy() and protect() just from 
> the 
> descriptions.  In particular, this is what protect()'s docs say: 
>
> '' 
> The protect function adaptor can be used to make a bind expression be 
> treated 
> as a normal function instead. Both bind and lazy eargerly evaluates nested 
> bind expressions. The protect adaptor masks the type so bind or lazy no 
> longer 
> recognizes the function as bind expression and evaluates it. 
> '' 
>
> I don't know what "masks the type" means here.  This really needs an 
> example. 
>

Protect works just like protect in Boost.Bind, but I do agree I need an 
example. 

>
>   * Tests 
>
> I did not run the tests, and just sort of spot-checked them.  They seem a 
> bit 
> thin.  In particular, one of the ones I looked at was conditional().  This 
> one 
> seems like it is needs more coverage, including thorny ones that must be 
> kept 
> working when maintenance occurs.  There are probably others adapters that 
> need 
> similar treatment. 
>

What kind of test cases are missing for in conditional?
 

>
> Also, consider this -- Steven pointed out this as a failure case: 
>
> boost::fit::conditional(identity(), identity())(0); 
>
> and Paul says that it is supposed to fail, and that this failure is a 
> feature, 
> not a bug.  If so, a minimal use like this that fails to compile needs to 
> be 
> added as a fail-compile test (and of course this intention should be 
> documented). 
>

Actually, Steven has convinced me to change this. I can see now why this is
useful. A compile-fail will probably still be useful for the `match` 
adaptor.
 

>
>
>   * Usefulness 
>
> I think the library is very useful.  There seem to be some things in there 
> that use of which might be deemed a bit clever (not the good kind), like 
> infix(), but then there are things like conditional() that I can see being 
> *extremely* useful. 
>
>
> - Did you attempt to use the library? If so: 
>
> Yes. 
>
>   * Which compiler(s) 
>
> Clang 3.6 
>
>   * What was the experience? Any problems? 
>
> I found I could gin up quick uses of things like conditional() (the most 
> interesting adaptor for my purposes) in a short period of time.  It was 
> quite 
> straightforward. 
>
> - How much effort did you put into your evaluation of the review? 
>
> About 5 or 6 hours. 
>

Thanks, Zach for your review.
 

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