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