[boost] [Fit] Louis Dionne's formal review

Subject: [boost] [Fit] Louis Dionne's formal review
From: Louis Dionne (ldionne.2_at_[hidden])
Date: 2016-03-11 17:32:21

First of all, sorry for submitting my review so late.

> - Whether you believe the library should be accepted into Boost

Yes, conditionally.

> - Your name

Louis Dionne

> - Your knowledge of the problem domain.

Extensive. In fact, I have implemented something very similar to Fit in
parallel to Paul, but in the Hana library. Hence, I understand what Fit
can be useful for, and I know many of the challenges Paul must have faced
in designing the library.

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

Good. It is simple and quite lean, which is nice. However,

- The issue about `capture`'s semantics (and `pack`'s semantics too) ought to
  be addressed. I created an issue for this:
  https://github.com/pfultz2/Fit/issues/141.

- It would be nice to provide a master header:
  https://github.com/pfultz2/Fit/issues/143

- I am not sure that `alias` has its place as part of the public interface of
  the library. I have only looked at it quickly, but it seems to diverge pretty
  heavily from the purpose of the rest of the library. I know it's used to
  implement `fit::pack`, but is it necessary to make it part of the public
  interface? Zach Laine already opened a similar issue here:
  https://github.com/pfultz2/Fit/issues/95

- I'm not sure about `compress` and `reverse_compress`. I think `fold` and
  `reverse_fold` (or my favorite `fold_left` and `fold_right`) would be
  better suited. An issue was already filed by Vicente here:
  https://github.com/pfultz2/Fit/issues/62

>    * Implementation

I have not looked at the implementation intensively, but I have to say that
the profusion of macros is a bit off-putting. That being said, it's probably
difficult to do better while supporting archaic compilers such as GCC 4.6.
Hats off to Paul on this one.

Also, I think the `fit/detail/ref_tuple.hpp` is unused and should be removed.

Finally, why do you use your own `move` function? Is it because it is not
marked constexpr in pre-C++14?

>    * Documentation

I submitted pull requests to fix typos, but these are not worth mentioning
here. Comments that might be of interest for the purpose of this review were
made in the form of GitHub issues:

- https://github.com/pfultz2/Fit/issues/131
- https://github.com/pfultz2/Fit/issues/132
- https://github.com/pfultz2/Fit/issues/133
- https://github.com/pfultz2/Fit/issues/135
- https://github.com/pfultz2/Fit/issues/136
- https://github.com/pfultz2/Fit/issues/138
- https://github.com/pfultz2/Fit/issues/139
- https://github.com/pfultz2/Fit/issues/140
- https://github.com/pfultz2/Fit/issues/142
- https://github.com/pfultz2/Fit/issues/144

>    * Tests

I have looked quickly, and basically all the individual components seem to be
tested. It's hard to evaluate the depth in which each component is tested, but
it seems OK. Also, the fact that the library is tested on Travis CI on each
commit is a blessing, and it makes me much more confident about its quality.

>    * Usefulness

This, I think, is the biggest problem of the library. I personally know very
well why the library is useful, but I can also understand that someone with
a different background would not feel the same. In my view, the library is
mostly useful as a replacement to shortcomings of lambdas in C++11/14. Not
only this, but in large part yes.

Indeed, let's say I have a function object `f`, and I'd like to get a function
object that swaps the arguments and calls `f`. The obvious solution (simplified)
is

    auto g = [f](auto x, auto y) { return f(y, x); };

In other words, the simplest way to express my intent is to use a lambda and
to do exactly what I mean. Unfortunately, this has several limitations

- The lambda above can't be constexpr
- The lambda can't appear in an unevaluated context

So I end up using `fit::flip`. In a different situation, I might want to
capture many variables in a lambda, but moving them into the capture (not
copying). So I write

    auto f = [x = std::move(x)...](auto const& param) {
        ...
    };

but unfortunately there's no way to capture a parameter pack by moving each of
its elements in a lambda. So I resort to using `fit::capture`.

Examples like this abound, and IMO these limitations are the main (but not the
only) reason for using Fit. Actually, this is how I implemented a library
similar to Fit inside Hana; I started by using lambdas everywhere and then
I wanted to make everything constexpr, capturing by move and things like
that.

All of this is very useful, but I feel like the documentation completely
misses the goal of explaining the problem solved by Fit (the problem I
would use it for, at least). This failure of motivating its own existence
is the main reason why I think Fit should be accepted conditionally.

> - Did you attempt to use the library? If so:
>    * Which compiler(s)

AppleClang 6.1.0.6020053 (shipped with Xcode 6.4)
Clang 3.5, 3.6, 3.7

I used both the Makefile and the Ninja generators for CMake, and both worked.

>    * What was the experience? Any problems?

Classic CMake setup, everything works well and the tests compile and run fast.
No problem whatsoever, as expected.

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

A couple of hours for the formal review itself, but I have been looking at
Fit's development for a while.

Summary
-------
Fit should be accepted conditionally and re-submitted for a mini-review before
it can be accepted. My conditions are

1. Correctly justify the purpose of the library
2. Resolve the issues on `capture` and `pack`'s semantics

I think all the other points I raised are important too, but they shouldn't
refrain Fit from being accepted. I also trust that Paul would be willing to
continue working on the issues that were raised during this review if Fit
were to be accepted.

Regards,
Louis