[Fit] Louis Dionne's formal review
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
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
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
Paul Fultz II <pfultz2 <at> yahoo.com> writes:
[...]
Thanks Louis for the review. Do you have any feedback/thoughts about the compile-time performance of the library?
It's hard to tell without doing benchmarks, which I didn't do. Things I might suggest benchmarking would be creating large `fit::pack`s and `fit::capture`s, using `compress` with many arguments and using `fit::arg` with many arguments. Just looking at the implementation quickly, it seems OK. For example `pack` does not use a recursive implementation and all the functions I looked at used flat variadic expansion whenever possible instead of recursion. The C++11/14 way, basically. However, `detail::seq` is implemented using recursion. It's not dramatic, but you could use a logarithmic approach instead to avoid hitting template instantiation depth limits. Regards, Louis
On Saturday, March 12, 2016 3:38 PM, Louis Dionne <ldionne.2@gmail.com> wrote:
Paul Fultz II <pfultz2 <at> yahoo.com> writes:
[...]
Thanks Louis for the review. Do you have any feedback/thoughts about the compile-time performance of the library?
It's hard to tell without doing benchmarks, which I didn't do. Things I might suggest benchmarking would be creating large `fit::pack`s and `fit::capture`s, using `compress` with many arguments and using `fit::arg` with many arguments.
Just looking at the implementation quickly, it seems OK. For example `pack` does not use a recursive implementation and all the functions I looked at used flat variadic expansion whenever possible instead of recursion. The C++11/14 way, basically. However, `detail::seq` is implemented using recursion. It's not dramatic, but you could use a logarithmic approach
instead to avoid hitting template instantiation depth limits.
Thats a good idea, I opened an issue to change this.
Regards, Louis
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Le 11/03/2016 23:32, Louis Dionne a écrit :
First of all, sorry for submitting my review so late. Thanks Louis for your review, which it is not late.
Thank also for creating the github issues. We have yet all the weekend to work on FIt review. Best, Vicente
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.
participants (4)
-
Louis Dionne -
paul Fultz -
Paul Fultz II
-
Vicente J. Botet Escriba