Boost logo

Boost :

Subject: Re: [boost] [Fit] Review
From: Paul Fultz II (pfultz2_at_[hidden])
Date: 2016-03-09 19:12:08


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, construct<std::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
>


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk