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

Subject: Re: [boost] [Fit] Louis Dionne's formal review
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2016-03-12 03:36:18

Le 12/03/2016 00:37, Paul Fultz II a écrit :
>
> On Friday, March 11, 2016 at 4:32:53 PM UTC-6, Louis Dionne wrote:
>>
>> 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.
>>
Yes I remember that we talked about Fit during Hana's review.

>>> 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.
Agree this needs to be resolved.
>> - It would be nice to provide a master header:
>>    https://github.com/pfultz2/Fit/issues/143
+1
>> - 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
>> <https://www.google.com/url?q=https%3A%2F%2Fgithub.com%2Fpfultz2%2FFit%2Fissues%2F95&sa=D&sntz=1&usg=AFQjCNEz3FbTR0JU7D15JwclPfanh3YsXg>
>
> I use this in another library to implement tuples. However, maybe I should
> leave alias undocumented for now. It is the one thing in the library that
> doesn't really fit(no pun intended).
>   
If this is not used internally and is not related,  why would  you need 
it in Fit? I believe its is better to remove it.
>>
>> - 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
>
> I always thought `fold` might be kind of strange since there is no sequence,
> but other people don't see it that way, so I think I will rename those to
> `fold`.
>   
+1 for fold. We can think it applies on the pack sequence.

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.
>>
> Well the macros are needed for MSVC as well.
At the beginning I believed that Fit was a C++14 library. Wondering if 
it is worth maintaining two different sources.
>   
>
>> Also, I think the `fit/detail/ref_tuple.hpp` is unused and should be
>> removed.
+1

https://github.com/pfultz2/Fit/issues/145
>> Finally, why do you use your own `move` function? Is it because it is not
>> marked constexpr in pre-C++14?
>>
> Yes thats why.
This is a common issue for C++11/C++14 libraries. Shouldn't we move this 
to some common library?

https://github.com/pfultz2/Fit/issues/146
>   
>
>>
>>>     * 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.
>>
> I think I was trying to address a much larger usefulness, because people may
> not understand why using lambdas in this way is useful, but I think it is
> something important I should discuss in the documentation as well.
Agreed.

https://github.com/pfultz2/Fit/issues/33
>   
>
>> 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.
>>
> Interesting, I have never thought about explaining the motivation as
> overcoming the shortcomings of lambdas. I'll try to discuss that more in
> the documentation.
>   

IMHO, one of the valid motivations of using macros at the interface level is to overcome missing language features. Some will surely remember when we had macros to emulate exception handling.

https://github.com/pfultz2/Fit/issues/33
  
Best,
Vicente