[boost] [Fit] Review

Subject: [boost] [Fit] Review
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2016-03-09 09:49:38

- Whether you believe the library should be accepted into Boost

Yes, conditionally.

  * Conditions for acceptance

I think the documentation and naming issues I raise below, and those filed
on
GitHub by me and others should be addressed before acceptance.  Also, the
ADL
issues that Steven Watanabe alluded to previously need to be fixed.  After
that, I think it should be re-submitted.

- Your name

Zach Laine

- Your knowledge of the problem domain.

I do generic programming regularly, and have done for years.

You are strongly encouraged to also provide additional information:
- What is your evaluation of the library's:
  * Design

I think it's good.  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()?

compress() is defined in terms of "fold", with a link to the Wikipedia page
for same; why not call it foldl()?  Similarly, reverse_compress() should be
foldr().

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

  * Implementation

I only briefly looked at the implementation, and only a few adaptors at
that.
What I saw did not leave me any concerns in particular.

However, I *am* concerned by Steven Watanabe's claim that the library is
ADL-unsafe.  He did not state exactly why that I could tell, but this needs
to
be addressed if so.

  * Documentation

The Quick Start section is good at showing a compelling motivating case for
using the library.  The result at the end of the section looks like a very
good start on a "Scrap Your Boilerplate" solution for simple, dump-style
printing.  It's relatively easy to read and write.

The documentation is what needs the most work, from what I've seen.  The
Quick
Start is quite nice, but then there are some things that are underexplained
in
the subsequent Overview and Basic Concepts sections.  For instance, why are
these macros used everywhere and what do they do?  They should be used for
the
first time after showing what the expanded code looks like.  This will
justify
their use for those users that want them, and show those that don't what
they
can write instead.

Others have written during the review that they don't understand why this
library is useful.  I think this is simply a documentation failure.  I find
the printing example from the Quick Start compelling, but perhaps Paul could
show how it would have to be done if hand-written (even if only partially).
Also, Paul has told me offline that there are a small number of primitive
operations on which everything else is built (pack, unpack, and apply,
IIRC).
Perhaps show how those can be used to implement the other adapters, and how
hard they are to write yourself.  Showing how at least some of the library
can
be used as fundamental building blocks would go a long way.

Some specifics.  I have read the online docs before, and I put several bugs
into the GitHub page for what I consider insufficient examples or
explanations.  Some new points, though:

I think alias() is in bad need of a motivating example.

I don't understand the distinction between lazy() and protect() just from
the
descriptions.  In particular, this is what protect()'s docs say:

''
The protect function adaptor can be used to make a bind expression be
treated
as a normal function instead. Both bind and lazy eargerly evaluates nested
bind expressions. The protect adaptor masks the type so bind or lazy no
longer
recognizes the function as bind expression and evaluates it.
''

I don't know what "masks the type" means here.  This really needs an
example.

  * Tests

I did not run the tests, and just sort of spot-checked them.  They seem a
bit
thin.  In particular, one of the ones I looked at was conditional().  This
one
seems like it is needs more coverage, including thorny ones that must be
kept
working when maintenance occurs.  There are probably others adapters that
need
similar treatment.

Also, consider this -- Steven pointed out this as a failure case:

boost::fit::conditional(identity(), identity())(0);

and Paul says that it is supposed to fail, and that this failure is a
feature,
not a bug.  If so, a minimal use like this that fails to compile needs to be
added as a fail-compile test (and of course this intention should be
documented).

  * Usefulness

I think the library is very useful.  There seem to be some things in there
that use of which might be deemed a bit clever (not the good kind), like
infix(), but then there are things like conditional() that I can see being
*extremely* useful.

- Did you attempt to use the library? If so:

Yes.

  * Which compiler(s)

Clang 3.6

  * What was the experience? Any problems?

I found I could gin up quick uses of things like conditional() (the most
interesting adaptor for my purposes) in a short period of time.  It was
quite
straightforward.

- How much effort did you put into your evaluation of the review?

About 5 or 6 hours.

Zach