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: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2017-09-11 16:55:13

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.

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()?

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

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

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

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

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_()".

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

>   - Did you try to use the library? With which compiler(s)? Did you
>     have any problems?
>

I did this quite a bit the first time around, and it was very easy to use
the part I care about the most (conditional()), using Clang 3.6.  The same
code works fine with Clang 4, but I did not do anything with the new
version of the library more than what I did with the old one.

  - How much effort did you put into your evaluation? A glance? A quick
>     reading? In-depth study?
>

I spent 5-6 hours on the first review, and about 2 hours this time.

>   - Are you knowledgeable about the problem domain?
>

Yes.  I do a lot of generic programming.

>   - Were the concerns from the March 2016 review of Fit addressed?
>

 Many of them were.  The only ones that were not were related to naming, as
repeated above.  As I said, I don't consider any of those naming choices to
be blockers, just suboptimal.

Zach