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

Subject: Re: [boost] [Fit] Louis Dionne's formal review
From: Paul Fultz II (pfultz2_at_[hidden])
Date: 2016-03-11 18:37:14

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

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

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

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

Yes thats why.
 

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

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.
 

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

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

Thanks Louis for the review. Do you have any feedback/thoughts about the
compile-time performance of the library?
 

>
> Regards, 
> Louis 
>
>
>
> _______________________________________________ 
> Unsubscribe & other changes: 
> http://lists.boost.org/mailman/listinfo.cgi/boost 
>