Subject: Re: [boost] [Fit] Review
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2016-03-09 18:28:56
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
but he has created an issue in github.
https://github.com/pfultz2/Fit/issues/123 -Conditional should accept
mutiple functions of the same type
> - 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
> GitHub by me and others should be addressed before acceptance. Also, the
> 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
> Fit, as was discussed previously. I even consider this a feature, allowing
> 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
> 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
> flow() is a super obscure name! Why is this not reverse_compose() or
> indirect() could more easily be understood if it were called deref() or
> 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.
> find it too easy to confuse with if_().
> * Implementation
> I only briefly looked at the implementation, and only a few adaptors at
> 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
> 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
> Start is quite nice, but then there are some things that are underexplained
> 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
> first time after showing what the expanded code looks like. This will
> their use for those users that want them, and show those that don't what
> 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,
> 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
> 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
> descriptions. In particular, this is what protect()'s docs say:
> The protect function adaptor can be used to make a bind expression be
> 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
> recognizes the function as bind expression and evaluates it.
> I don't know what "masks the type" means here. This really needs an
> * Tests
> I did not run the tests, and just sort of spot-checked them. They seem a
> thin. In particular, one of the ones I looked at was conditional(). This
> seems like it is needs more coverage, including thorny ones that must be
> working when maintenance occurs. There are probably others adapters that
> 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
> 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
> * 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:
> * 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
> - How much effort did you put into your evaluation of the review?
> About 5 or 6 hours.
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk