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

Subject: Re: [boost] [Boost-users] Yap's formal review is starting now!
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2018-02-16 21:38:00

On Fri, Feb 16, 2018 at 2:25 PM, Brook Milligan via Boost <
boost_at_[hidden]> wrote:

>
> > On Feb 5, 2018, at 8:53 AM, Louis Dionne via Boost via Boost-users <
> boost-users_at_[hidden]> wrote:
>

[snip]

Thanks for reviewing, Brook!

> > * Design
>
> Overall, the design of Yap seems excellent, although I do agree with the
> comments regarding potential modifications to the expression concept.  In
> particular, I like Peter's idea of making the tag a type and the idea of
> making the expression template variadic (instead of using a tuple):
>
>   template<expr_kind kind> struct expression_tag {};
>
>   using plus_tag = expression_tag<expr_kind::plus>;
>
>   template < typename Tag, typename ... T > class expression;
>
> It seems to me that this would make matching expressions much more
> flexible, but perhaps I am missing something.  It might also some
> dispatching in the implementation to make things simpler?
>
> It is clear that the key to flexibility is the ability to write
> transforms, which is something I have not done a lot of.  Thus, it is
> crucial to get the expression concept right so that generic transforms are
> easy and matching specific expressions is also easy.  My intuition suggests
> the definition above would be good, but this should be hashed out
> carefully, and the rationale documented.
>

I mentioned elsewhere that the library started out this way, with a
top-level variadic interface.  It made everything more complicated for the
implementation adn the interface, and as Steven pointed out elsewhere, it
makes expr_ref very hard to get right.

[snip]

> * Documentation
>
> The documentation is the weakest point of Yap.  In particular, it does not
> establish (in my mind at least) a clear understanding of the rationale and
> design of the primary use case, which seems to be the evaluate(transform())
> idiom discussed on the list.
>
> Introduction: This needs to guide the reader to think about Yap idioms in
> the right way.  The motivation sections seems clear: we need Yap because
> expression templates are valuable but Proto is too complicated.  Perhaps,
> however, this needs to be recast a bit to avoid bringing in Proto at the
> first.  Perhaps a better motivation is (i) what are expression templates
> and why are they useful (briefly; refer to a more detailed section that
> develops this more completely) and (ii) what are some quick examples
> (really short code snippets, not necessarily complete programs).
>

There is a much fuller intro to the idea of expression templates
themselves, and how Yap represents them in particular, that I added to the
boot_review branch a night or two ago.  Based on what you wrote above, I'm
going to add another section about the idiomatic use of Yap right after
than, to laya  foundation for understanding the rest of the docs.

> The features are also clear, but only with a fair bit of background. It
> also seems that the features are not necessarily in an order that reflects
> either their importance or the way a user would encounter them.  A better
> list might be:
>

[snip]

Yeah, I was looking at that list with disdain, but was unsure how exactly
to fix it.  I like what you wrote here; I've put your detailed notes here
into a GitHub ticket.

> Tutorial: if there are examples that illustrate points in the tutorial,
> there should be links from the tutorial page to the corresponding example.
>
> Expressions: there is an example at the bottom of the page of a problem
> that recurs elsewhere.  The phrase "there are lots of examples of how to do
> this in the Examples section" is not helpful without references to which
> particular examples illustrate this point.  It should also be clear in the
> examples what points they are examples of.
>

Done.

> Kinds of expressions:  over the past year I have found myself consulting
> this page many times expecting a list of the kinds of expressions, i.e.,
> all the tags, etc.  Why is that not here?  Why does this only refer to
> "special" kinds?
>

This part confuses me a bit.  Right after the special ones, I had this:

"The other expression kinds are all the overloadable operators, See <link>
for the full list."

I've put the non-special ones first, with a couple of explicit examples.  I
really don't want to hit readers with the full list, because it's too much
info at that point, and I think people generally know what overloadable
operators are.  Does that work?

> Operators: comments like "you can write your own operators that return
> Expressions if you like" would be much more useful with the following: (i)
> an indication of when that would be appropriate (is this just another joke,
> as in "be a masochist, write your own", or is this "sometimes you
> legitimately need to and so you can"?) and (ii) examples of how to do
> this.  I believe there are similar points throughout the documentation to
> which analogous comments apply.  It would also be useful to have a link to
> all the operator macros here.
>

I don't understand.  The full sentence says "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."  There's a link to all the user
macros two sentences later.

> Explicit transformations: the phrases "that will be placed into result at
> the corresponding place in result" seems confusing.  Especially given the
> central role played by transforms, I feel that a much clearer description
> of their operation is needed.  This could leverage a detailed section on
> expression trees generally and (another one?) on the representation of them
> in Yap.  The latter is implicit in the expression concept, but clarity
> would be improved by making it explicit and by relating it to the more
> general concepts.
>

Yeah, I've never been super happy with that sentence.  I think this calls
for a picture, too.

> The sentence "Otherwise, N is used, and visitation of nodes in expr
> continues below N." seems misleading if I am understanding transforms
> correctly.  First, the "N is used" part is very vague; used for what
> specifically?  Second, the "visitation continues" part seems wrong.  Isn't
> the recursion entirely under the control of the transform?  If so, then how
> can this assertion be made?  Perhaps you mean something like "sane
> transforms should often be written to have this property".  Again, this
> gets at the need to provide much clearer best practices information that is
> clearly distinct from tutorials and reference material.  These all serve
> different needs and should not be interspersed in the documentation.
>

The "N is used" and subsequent traversal part is right, it's just badly
explained.  The idea is that if N is visited and no transform overload
applies to it, it is copied into the result, and transform() goes into its
children.  Needs a rewrite, it seems.

> The tag and expression forms of matching in transforms are mentioned on
> this page, but documented elsewhere.  This is difficult organization.
> Reorganize this to bring the conceptually related parts together.
>

Agreed.  This is already a lot better on the review branch.

> Evaluating expressions: Given the direction of the discussion (i.e.,
> dropping customization points), it seems that this will be reduced to
> "Boost.YAP expressions are explicitly evaluated by calling the
> evaluate()".  It seems, however, that this could be a good point to include
> information on (i) evaluation of operators follows native semantics for the
> types involved, (ii) the value of local reasoning about semantics, (iii) a
> brief discussion of equivalencies and differences between the underlying
> native code and what is evaluated by evaluate().
>

I'll make this more explicit.

> Operator macros: I think this makes more sense in "Operators".
>

Again, I don't want to blast the reader with all of this before they need
it.  I think the link in the earlier section is enough if they want to look
at it at that point.

> How expression operands are treated: most of the information here (i.e.,
> not the table and specifics relating to is) seem more appropriate in a
> section dealing with the design of Yap expressions and the implementation
> of them.  This is trying to motivate the correspondence between C++
> expressions and Yap expression trees; that needs unification elsewhere.
> The last bit is more about specifics and should be reference material.
>

Agreed.  Much of this will go into the new section I mentioned at the top.

> Customization points:  I assume this will disappear.
>

Yes.

> Transform matching: this information needs to be integrated into a unified
> description of transforms.
>

Agreed.  This is done on the review branch.

Reference: why is the reference organized around the header files?  I feel
> it would make more sense to organize reference information around concepts,
> classes, functions, and the like.  The headers are details that a reader
> generally does not care about as an index to finding information.  They are
> clearly needed as part of the reference documentation for each class, etc.,
> but that information is provided on those pages.
>

It isn't, or at least not intentionally so.  he fact that the header
organization is at the top is a mistake that's I've fixed.

> Index: the index should have links to the reference information for
> functions and classes.  It seems that one must go through the header file
> links to get them (because not everything is linked here), but that does
> not match my expectation as a reader.  Conversely, everything linked to by
> the header file pages should be linked in the index.  Indeed, those pages
> are what appear to me to be the primary reference documentation, but they
> are needlessly difficult to navigate to.
>

That's just how the QuickBook indexing + Doxygen system works, as far as I
know.  I'm using the auto-indexing.

[snip]

Zach