On Wednesday, March 9, 2016 at 8:50:01 AM UTC-6, Zach Laine wrote:
- 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()?
I chose `by` because of how it is used linguistically. That is you write: `sort(v.begin(), v.end(), by(&Employee::Name, _<_))` which I read "sort by employee name". Also, if I write `by(decay, constructstd::tuple())`, I read "construct tuple by decay". In haskell, it uses `on` for this adaptor, which is what I originally called it. `project` is a more straighforward name.
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().
Not `foldr`. This is because `foldr` is symetrical. For example, `foldl` and `foldr` should produce the same results: foldl(_+_, std::string())("Hello", "-", "world"); // "Hello-world" foldr(_+_, std::string())("Hello", "-", "world"); // "Hello-world" However, compress and reverse_compress work like this: compress(_+_, std::string())("Hello", "-", "world"); // "Hello-world" reverse_compress(_+_, std::string())("Hello", "-", "world"); // "world-Hello" I was reluctant to call it `fold` as it seems to imply some data structure to fold over, whereas this is simply an adaptor. I used the word compress as its a another name used for fold. However, it seems more people would prefer to use `fold` and `reverse_fold`.
flow() is a super obscure name! Why is this not reverse_compose() or similar?
I actually got the name from underscore.js. The reason why I chose this name instead of `reverse_compose` is because when this is used with pipable functions it seems confusing: reverse_compose( filter([](int i) { return i < 3; }), transfrom([](int i) { return i*i; }) )(numbers); With the word 'reverse' there, it almost looks as if it computes the pipeline in reverse order, which it doesn't. So I would prefer a name without reverse in it.
indirect() could more easily be understood if it were called deref() or dereference().
Well, `indirect` is how it is commonly called in libraries such as Boost.Range, range-v3, and PStade libraries. So I would like to keep the name consistent with other similar constructs.
I don't feel as strongly about partial(), but I think it might be clearer if it were called partial_apply() or curry().
Hmm, I don't think a name with `apply` in it is good name for an adaptor.
conditional() should be called invoke_first(), call_first_of(), or similar. I find it too easy to confuse with if_().
I see, I was trying to describe an adaptor where you could put functions with conditions in it. Other people, seem to prefer a name like `linear`, so I might use that instead.
* 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.
I don't think the docs should show the expansion of the macros, that is part of the implementation and not interface. I could show an "idea" what is is expanding to, with explanation of what else it is doing beyond the simple explanation.
This will justify their use for those users that want them, and show those that don't what they can write instead.
I could show an alternative without the macros, but I would prefer to put that in the Advance section. In the introduction of the library, I would rather show the constructs that can be easily used without a need to explain a bunch of caveats.
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.
Protect works just like protect in Boost.Bind, but I do agree I need 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.
What kind of test cases are missing for in conditional?
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).
Actually, Steven has convinced me to change this. I can see now why this is useful. A compile-fail will probably still be useful for the `match` adaptor.
* 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.
Thanks, Zach for your review.
Zach
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost