Le 09/03/2016 15:49, Zach Laine a écrit : Thanks Zach for your review, I will just comment for the moment the following. I don't remember if Paul has confirmed in this ML that the following should work boost::fit::conditional(identity(), identity())(0); but he has created an issue in github. https://github.com/pfultz2/Fit/issues/123 -Conditional should accept mutiple functions of the same type Best, Vicente
- Whether you believe the library should be accepted into Boost
Yes, conditionally.
* Conditions for acceptance
I think the documentation and naming issues I raise below, and those filed on GitHub by me and others should be addressed before acceptance. Also, the ADL issues that Steven Watanabe alluded to previously need to be fixed. After that, I think it should be re-submitted.
- Your name
Zach Laine
- Your knowledge of the problem domain.
I do generic programming regularly, and have done for years.
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design
I think it's good. I particularly like the fact that it attempts to keep everything constexpr- and SFINAE-friendly wherever possible.
I *really* like reveal() and failure(). Too few TMP-heavy libraries pay attention to providing good error messages.
I have no problem with the constness requirements on Callables used throughout Fit, as was discussed previously. I even consider this a feature, allowing me to detect non-const Callables, which are almost always an error on my part. This is especially true since std::ref() and mutable_() let me explicitly choose how to handle mutable Callables, in a way that is also explicit at the call site.
There are a number of adaptors that I find have obscure names:
by() is a projection; why not call it project()?
compress() is defined in terms of "fold", with a link to the Wikipedia page for same; why not call it foldl()? Similarly, reverse_compress() should be foldr().
flow() is a super obscure name! Why is this not reverse_compose() or similar?
indirect() could more easily be understood if it were called deref() or dereference().
I don't feel as strongly about partial(), but I think it might be clearer if it were called partial_apply() or curry().
conditional() should be called invoke_first(), call_first_of(), or similar. I find it too easy to confuse with if_().
* Implementation
I only briefly looked at the implementation, and only a few adaptors at that. What I saw did not leave me any concerns in particular.
However, I *am* concerned by Steven Watanabe's claim that the library is ADL-unsafe. He did not state exactly why that I could tell, but this needs to be addressed if so.
* Documentation
The Quick Start section is good at showing a compelling motivating case for using the library. The result at the end of the section looks like a very good start on a "Scrap Your Boilerplate" solution for simple, dump-style printing. It's relatively easy to read and write.
The documentation is what needs the most work, from what I've seen. The Quick Start is quite nice, but then there are some things that are underexplained in the subsequent Overview and Basic Concepts sections. For instance, why are these macros used everywhere and what do they do? They should be used for the first time after showing what the expanded code looks like. This will justify their use for those users that want them, and show those that don't what they can write instead.
Others have written during the review that they don't understand why this library is useful. I think this is simply a documentation failure. I find the printing example from the Quick Start compelling, but perhaps Paul could show how it would have to be done if hand-written (even if only partially). Also, Paul has told me offline that there are a small number of primitive operations on which everything else is built (pack, unpack, and apply, IIRC). Perhaps show how those can be used to implement the other adapters, and how hard they are to write yourself. Showing how at least some of the library can be used as fundamental building blocks would go a long way.
Some specifics. I have read the online docs before, and I put several bugs into the GitHub page for what I consider insufficient examples or explanations. Some new points, though:
I think alias() is in bad need of a motivating example.
I don't understand the distinction between lazy() and protect() just from the descriptions. In particular, this is what protect()'s docs say:
'' The protect function adaptor can be used to make a bind expression be treated as a normal function instead. Both bind and lazy eargerly evaluates nested bind expressions. The protect adaptor masks the type so bind or lazy no longer recognizes the function as bind expression and evaluates it. ''
I don't know what "masks the type" means here. This really needs an example.
* Tests
I did not run the tests, and just sort of spot-checked them. They seem a bit thin. In particular, one of the ones I looked at was conditional(). This one seems like it is needs more coverage, including thorny ones that must be kept working when maintenance occurs. There are probably others adapters that need similar treatment.
Also, consider this -- Steven pointed out this as a failure case:
boost::fit::conditional(identity(), identity())(0);
and Paul says that it is supposed to fail, and that this failure is a feature, not a bug. If so, a minimal use like this that fails to compile needs to be added as a fail-compile test (and of course this intention should be documented).
* Usefulness
I think the library is very useful. There seem to be some things in there that use of which might be deemed a bit clever (not the good kind), like infix(), but then there are things like conditional() that I can see being *extremely* useful.
- Did you attempt to use the library? If so:
Yes.
* Which compiler(s)
Clang 3.6
* What was the experience? Any problems?
I found I could gin up quick uses of things like conditional() (the most interesting adaptor for my purposes) in a short period of time. It was quite straightforward.
- How much effort did you put into your evaluation of the review?
About 5 or 6 hours.
Zach
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost