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@lists.boost.org> 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