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/
> > 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:
Â Â Â Â Â Â Â Â 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
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
> 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:
> > Â - What is your evaluation of the implementation?
> I did not look at it much, but what I did look at had no readily apparent
> > Â - 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk