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