On Sun, Feb 11, 2018 at 5:53 PM, Fletcher, John P
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