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