Re: [boost] [Fit] Review

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

        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
>