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 <
> 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.
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
indirect() could more easily be understood if it were called deref()
I don't feel as strongly about partial(), but I think it might be clearer
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
> - 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.