Re: [boost] Yap's formal review is starting now!

Subject: Re: [boost] Yap's formal review is starting now!
From: a.hagen-zanker_at_[hidden]
Date: 2018-02-14 01:07:12

> We encourage your participation in this review. At a minimum, please state:

> - Whether you believe the library should be accepted into Boost

Not without major improvement of the documentation
 
> - Your name

Alex Hagen-Zanker

> - Your knowledge of the problem domain

I have used Expression Templates in my own work. Notably, I am not familiar with Proto, which is a hindrance in reading the documentation

> - What is your evaluation of the library's:
>  * Documentation
> * Design

I have only looked at the documentation

>- Did you attempt to use the library? If so:
  
No

This is a library with great potential. Expression Templates indeed are rad. However, if you want people to use the library, you need to take more care in documenting its purpose and functionality. 

I went through the documentation page-by-page and mainly ran into confusion. Please see my notes below:

* Boost YAP review notes

The documentation could be a lot more accessible. First of all it would be good if you could limit the references to Proto, this is not helpful for users who don't know Proto. Perhaps one separate page where you list the difference between Proto and Yap would be useful. Next, remove all the jokes. Some are funny and others not, but none are helpful. And when trying to get my head around how to use the library, it is annoying to read the same joke again-and-again. (Yes, it took me many reads to understand what the library is offering exactly). The bloggy casual style is annoying me, when I am trying hard to make sense of the library. 

Don't use examples without explaining what they are an example of. 

It would be good if the documentation page could include a definition / explanation of what an expression template is. Actually, I think you need to define what an expression is in this context. In my book, expressions are not just made of operators, but can also include function calls. However, the implicit impression I get from the YAP documentation is that you only consider expressions based on operators, and not functions. For instance, I consider std::sqrt(x) an expression. However, looking at expr_kind (https://tzlaine.github.io/yap/doc/html/boost/yap/expr_kind.html) it seems that this kind expression is not supported, (but it could be as a Transform).

*Section Compiler Support
I don't think it is acceptable to produce a library that cannot be compiled in MSVC own front-end.I can see that Hana got away with it on the premise of being ground-breaking and pushing the boundaries. However, YAP is not trying to be groundbreaking, it is offering stuff that Proto already offered and generalizing functionality of Eigen, UBLAS and others. Can you really not support MSVC? What is the bottleneck, and did you try to work around it?

*Section Dependencies
Why depend on Hana, my impression of Hana and the associated review / discussion is that this library is made for demonstration of next generation C++ rather than actual use. 

*Section Header Organisation
"If you want to ensure that you don't accidentally use expression<> (which I recommend),"

I suppose you mean to recommend not using expression<>, WHY do you offer it? At this stage it does not make sense to talk about a thing I could do, but rather shouldn't. You haven't told me anything yet and are confusing me with expression<>: it is there but don't use it... How would I 'accidentally ' use it.

*Section Expressions
https://tzlaine.github.io/yap/doc/html/boost_yap__proposed_/manual/tutorial/expressions.html 

The language used in the documentation could be much more precise. Superfluous qualifications can be removed and at times detail can be added. 

"Boost.YAP consists of expressions and functions that operate on them. Any type that models the Expression concept will work with any of the functions in Boost.YAP that takes an expression."

Could be reduced to: "Many functions in YAP operate on types that model the Expression concept." 

"For a type T to model the Expression concept, T must contain at least an expr_kind (terminal, plus-operation, etc.) and a boost::hana::tuple<> of values. That's it. This means that user-defined templates modeling ExpressionTemplate are very straightforward to make."

The mention of ExpressionTemplate comes out of nowhere. You were explaining Expressions, not ExpressionTemplates.

Could be reduced to:
"The Expression concept requires: 
1. an expr_kind member that indicates the kind of expression (terminal, plus-operation, etc.) 
2. a boost::hana::tuple<> member which contains the values upon which the expression operates."

with a separate line:
 
"The ExpressionTemplate concept requires a template class that models the Expression concept and takes both the expr_kind member value and the boost:::hana::tuple<> type as template arguments".

"Ok, so it's not that interesting by itself -- it has no operators. But we can still use it with the Boost.YAP functions that take an Expression. Let's make a plus-expression manually: "

This sentence makes no sense to an uninformed reader. Why would an Expression need to have operators, you said all it needs is an expr_kind and a bh::tuple? What do you mean by make a "plus-expression manually"? Is there another way? It has not been introduced yet. You then use yap::make_terminal and yap::make_expression functions that have not been introduced. Before, you said that functions take an Expression as input, but evidently these functions do not.

The note about expression<> does not make sense for an uninformed reader at this stage.
The note about functions working on Expressions and not ExpressionTemplates seems obvious and does not address a real concern.
the note about "one more thing" BOOST_YAP_CONVERSION_OPERATOR_TEMPLATE (a dubious solution),leaves me thoroughly confused. Why did you create this macro if it is a dubious solution? Please don't provide us with dubious solutions. Definitely don't do so not at this stage (still having to explain what Yap does).

*Section Kind of Expression
When you write: "A terminal contains a non-Expression value" I suppose you mean that "the bh::tuple of a terminal does not include Expression types", because an expression is a type not a value and bh::tuple of Expression is also a non-Expression (bh::tuple does not have an expr_kind member)/

*Section Operators
Starts with an example, without clarifying what it is an example of.
You write "As shown here, that sort of operator can be mixed with normal, non-lazy ones". I don't see this at all. Nothing has been shown here. There is a macro, but it is not clear what it is doing. I don't know what this "sort of operator" is and I don't know what a normal non-lazy operator is. I don't understand what you mean by mixed: what is being mixed with what?
You use this macro BOOST_YAP_USER_BINARY_OPERATOR_MEMBER, but don't bother explaining what it does.
You then write "Use of the macros is not necessary (you can write your own operators that return Expressions if you like), but it is suitable 99% of the time. " So when is it suitable and when not? If I run my program 1000 times will it give the wrong answer 10 times?

*Section Explicit Transformation
You use graph terminology (breadth first, node), but you have not yet explained what the graph is, what are the nodes and what are the edges? Is a node each member of the bh::tuple and when the member is an Expression, each member of its bh::tuple. You say that transform visits each node, but do not say what happens when a node is visited. The note on this page seems very informal. Can you explain in more precise terms? The note about generic code does not make sense to me. Need more explanation

"A Boost.YAP transformation is free-form; it must return a value, but may do just about anything else. It can transform an expression into anything -- a new expression of any kind, or even a non-expression value (effectively evaluating the expression). For example, here is the get_arity transform from the Calc3 example: "

I see that a xform can do anything to a type that models expression. this open-endedness is confusing. What is it that xform does.?

*Section Implicit Transformation
 This section has no content and refers to other parts of doc

"Implicit evaluation can apply when BOOST_YAP_CONVERSION_OPERATOR_TEMPLATE is defined. Note that this macro only changes the definition of expression<>, though. Your custom expression templates will not be affected. See the assignment to d1 in Lazy Vector for an example." 

OK, you already told me not to use expression<> , so BOOST_YAP_CONVERSION_OPERATOR_TEMPLATE  is of no interest to me.

*Section Evaluating Expression

Ok, I understand evaluation and evaluation_as. The bit about BOOST_YAP_CONVERSION_OPERATOR_TEMPLATE is highly confusing. It only pertends to expression<> which you already told me not to use. 

*section printing
- good

*Section Operator Macros
This section is cryptic, for instance  in the columns Use for BOOST_YAP_USER_UDT_UNARY_OPERATOR : "Free operators defined over non-Expression types constrained by a type trait (e.g. all std::map<>s)." What is a type constrained by a type_trait, and how is that related to std::map. 

* Section How Expression Operands Are Treated
This starts by talking about expression<> which you told me not to use. 

"Due to a limitation of Doxygen, each of the value(), left(), right(), and operator overloads listed here is a stand-in for three member functions." 
Doxygen is a tool, it should work for you. You should not let it dictate how you document the library.

Some parts of the documentation use boost::yap, others bost::yap

Did you consider Robert Ramey's advice on how to document libraries? https://www.youtube.com/watch?v=YxmdCxX9dMk   . I think it might be beneficial for yap.

I hope this is helpful. I won't manage to do a full review, so this is just based on the manual part of the documentation.

Alex