Re: [boost] [yap] Review

Subject: Re: [boost] [yap] Review
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2018-02-12 16:24:12

On Sun, Feb 11, 2018 at 5:53 PM, Fletcher, John P <j.p.fletcher_at_[hidden]>
wrote:

> I have quite a lot to say and I may not get it all down in one go.

[snip]

Hi, John.  Thanks for reviewing.

>  >  * Implementation

> I have encountered quite a lot of the implementation as I have worked on
> the examples.  I appreciate that this library makes extensive use of Boost
> Hana.  There are some places in the examples where the user needs to have
> an understanding of Boost Hana to work.  For example the transform example
> for arity uses Boost Hana maximum and transform.   I will except
> boost::hana::tuple from the comment as it is used so much and is similar to
> std::tuple.
>
> One issue I have had is with the namespace.  I am not clear why some code
> needs the full name e.g. boost::yap::evaluate   and in other places
> evaluate will do without using being defined.  It would be good if that was
> consistent.
>

Could you give an example?  I suspect that some cases where evaluate is
used are in library details where that name is in scope, and some are used
in places where I did not want a common name like evaluate() to resolve to
a different implementation due to ADL.

>  * Documentation
>
> I don't think that the documentation at present does full justice to the
> code.  There are several things which I found very useful which are not
> much mentioned in the code.

Please be as specific as you can.  This is an area that badly needs
attention, and which I have struggled with so far.  Outside perspectives
are always invaluable here.

>   When I saw that the examples were taken from work with proto I set out
> to make one which was missing, the mini_lambda example.  I have had a lot
> of success with that while having to dig quite hard to find information.
>
> make_expression_function
>
> I have found this very useful in moveing to a functional approach.  It is
> not much mentioned.
>

This was very easy to implement, and made near-parity with Proto a bit
easier to accomplish.  It's not emphasized simply because its a utility,
and doesn't match the main use case, which is:

auto expr = /* ... */;
auto expr_2 = yap::transform(expr, my_xform);
// however many more transforms...
auto result = yap::evaluate(expr); // or an evaluating transfom...

Anything that does not fit that main use case is generally given little
time in the docs.

[snip]

Thanks a gain for the review!

Zach