Re: [boost] [review][Fit] Review of Fit starts today : September 8 - September 17

Subject: Re: [boost] [review][Fit] Review of Fit starts today : September 8 - September 17
From: paul (pfultz2_at_[hidden])
Date: 2017-09-12 00:20:10

On Mon, 2017-09-11 at 11:55 -0500, Zach Laine via Boost wrote:
> On Fri, Sep 8, 2017 at 6:02 AM, Matt Calabrese via Boost <
> boost_at_[hidden]> wrote:
> 
> > 
> > A formal review of the Fit library developed by Paul Fultz II starts
> > today, September 8, and runs through September 17.
> > 
> > A branch has been made for the review. You can find Fit for review at
> > these links:
> > 
> > Source: https://github.com/pfultz2/Fit/tree/boost
> > Docs: http://pfultz2.github.io/Fit/doc/html/doc/
> > 
> [snip]
> 
> 
> > 
> > Please provide in your review whatever information you think is
> > valuable to understand your final choice of ACCEPT or REJECT including
> > Fit as a Boost library. Please be explicit about your decision.
> > 
> I vote to accept Fit.  I would like to see some naming changes, but I'm not
> making any of them conditions for acceptance, because I do not consider any
> of them to be deal-breakers.

Thanks, Zach, for the review.

> 
> Some other questions you might want to consider answering:
> > 
> > 
> > Â  - What is your evaluation of the design?
> > 
> (Still) good.  Here's what I wrote in the last review:
> 
> 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()?

by() is not a projection, rather it takes a projection.

I mainly named it for how it was used, so it would read more english-like:

std::sort(std::begin(people), std::end(people),
        by(&Person::year_of_birth, _ < _));

Like "sort by year_of_birth". In haskell, its called 'on', but it seems 'by'
is more natural for English.

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

Its because its used with pipable. So it can take a set of pipable functions
so they "flow" together. reverse_compose() could work, but I think the name
looks clunky as an alternative to pipable functions. 

Also, in haskell there is no reverse compose(just `.` for composition),
instead the `>>>` arrow operator can be used to indicate the direction of
composition, but there is no way to implement such an operator in C++.

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

This named after the indirect in the Ranges library. Also, I might add a
custom operator in the future, so calling it deref might not make sense.

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

Which there could be an uncurry function, but I've always understood curry to
be for one arg at a time.

Either waty, I could provide curry for repeated partial applications(ie what
partial is currently). Then provide partial and reverse_partial like hana for
a single partial application. I just wonder if this bloats the interface, as
we will have a lot of different functions for partial application.

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

I could call it first_of(). I do see the confusion with `std::conditional`.

> 
> For that matter, it would be nice if "if_()" were called "if_constexpr()",
> since that seems to match its semantics, and since there's a language
> feature that already does this; a reader of uses of "if_constexpr()" will
> get the intent faster than they would if they read "if_()".

To work like if_constexpr(), eval() would needed to be used. There are of
course other ways to use it(especially in point-free programming) that is not
an emulation of `if constexpr`, like how I implement `yield_if` here:

https://github.com/pfultz2/Hero/blob/master/include/hero/for_each.h#L43

> 
> > 
> > Â  - What is your evaluation of the implementation?
> > 
> I did not look at it much, but what I did look at had no readily apparent
> issues.
> 
> 
> > 
> > Â  - What is your evaluation of the documentation?
> > 
> It has gotten dramatically better since the last review.  The non-reference
> sections now provide a thorough tour of the library and its capabilities.
> 
> I took issue with some of the reference docs in the last review; those have
> all been addressed.
> 
> 
> > 
> > Â  - What is your evaluation of the potential usefulness of the library?
> > 
> I think the library is very useful.  For example, I had to do some somewhat
> hairy overload-resolution metaprogramming in an expression template library
> I've been working on, and I really wished conditional() were already in
> Boost so I could have used it instead.

Well, hopefully it will be there soon.

Thanks,
Paul