[boost] [Boost.Yap] Review results

Subject: [boost] [Boost.Yap] Review results
From: Louis Dionne (ldionne.cpp_at_[hidden])
Date: 2018-03-02 06:16:39

Dear Boost,

I am pleased to announce that the Yap library is CONDITIONALLY ACCEPTED
into Boost. I counted the following formal reviews:

- Alex Hagen-Zanker: REJECT / CONDITIONAL ACCEPT
- Barrett Adair: ACCEPT
- Brook Milligan: CONDITIONAL ACCEPT
- John Fletcher: ACCEPT
- Paul Fultz II: ACCEPT
- Steven Watanabe: ACCEPT

I'd like to thank everyone who participated to the review for their
feedback,
and also Zach Laine for his submission. The following is a summary of the
main issues that were raised during the review:

- Contextual evaluation of terminals
    The concern was that there was no way of evaluating an expression tree
    with a custom context other than to first transform it with a custom
    transform, and then evaluate it. This, however, requires encoding the
    recursion into the transform even when we only want to modify terminals.
    After a lot of discussion, everybody seemed to agree that the following
    would be desirable (paraphrasing Steven):

        `transform_evaluate` would behave exactly like `transform`, except
        that the default behavior for nodes that are not handled by the
        context is to evaluate the operators instead of building a new
        expression.

    Zach fixed that in https://github.com/tzlaine/yap/issues/63 by
introducing
    a new transform called `evaluation()` which evaluates terminals, and the
    ability to pass multiple transforms `t1`, ..., `tN` to `transform`. The
    first matching transform is executed, and `transform` will
copy-and-recurse
    if no transforms match. `transform_evaluate(expr, tform)` can then be
    implemented as `yap::transform(expr, tform, yap::evaluation())`.

- Auto-unwrapping of terminals in transform and automatic application of
  terminal transforms before unwrapping
    There was a concern that automatically unwrapping terminals in
transforms
    and applying terminal transforms before unwrapping would result in
counter
    intuitive behavior. After some discussion, it was determined that
automatic
    application of transforms before unwrapping could indeed lead to
surprising
    results and was removed. However, unwrapping of terminals is necessary
to
    keep the matching of terminals reasonable, so it was kept. I am
comfortable
    with that decision, as the unwrapping of terminals, which leads to a
very
    intuitive way of matching terminals, was one of the features that sprung
    to me as being a great usability boost when I first looked at the
library.

- Positional placeholders should stay or go?
    Some of the reviewers felt like it was not desirable to provide
positional
    placeholders as part of Yap, because those aren't the primary purpose of
    the library and C++14 obsoletes them with lambdas. Other reviewers felt
    like they were in fact very useful and shouldn't be removed. Since no
    clear consensus could be reached, no acceptance condition is attached to
    this issue. However, I personally favor the removal of placeholders from
    the core API on the basis of minimality. I would like to encourage Zach
    to check whether their removal would result in API simplifications; if
    so, I would encourage (but not require) their removal, at Zach's
    discretion.

- Documentation
    The documentation is not sufficiently extensive, and takes too much for
    granted from the reader. This was raised by a couple of reviewers and it
    has been my largest problem with the library so far. More on this below.

- The eval_xxx customization points.
    Nobody liked them, and they are already gone.

- BOOST_YAP_CONVERSION_OPERATOR_TEMPLATE
    Nobody liked it, and it is already gone.

- Short-circuiting of && and || in evaluate
    Steven brought up the fact that `yap::evaluate` would not short-circuit
    the evaluation of && and ||. This issue was mooted when customization
    points were removed. Also, the new `evaluation()` transform, which could
    suffer from the same short-circuiting problems, doesn't because it was
    implemented properly.

- Support for Boost.Build is missing.

- Hana vs not Hana
    Full disclosure: I am the author of Boost.Hana.
    A couple of reviewers raised concerns with Hana being part of the public
    interface. The main concern seemed to be about MSVC not being able to
    compile Hana, and hence Yap being unusable on MSVC. While I agree that
    MSVC is a big player, I believe that should not dictate what all Boost
    libraries do. Some libraries are meant to bridge gaps with older
compilers,
    some libraries are cutting edge. Yap is the latter, and that's fine. The
    MSVC folks are working hard on their C++ conformance, and they will
    eventually get there. Also, it is likely that Hana wouldn't be the
barrier
    anyway: Yap relies a lot on SFINAE and MSVC is known to be poor at it.
    Many of the features that one would likely want to use when using Yap
are
    also not too well supported by MSVC, so users would probably hit
limitations
    there anyway. Plus, as soon as MSVC is close enough, I'll start working
on
    Hana's side to support it.

    Taking my review manager hat off for a second, I'd also like to point
    out that Yap is exactly the kind of use case that Hana was built for.
    Just like MPL was (and is) used in many libraries' implementation and
    APIs, I don't see a problem with using Hana in Yap's API, since that
    is the right tool for the job.

    That being said, if Zach wishes to explore using Hana's concepts (like
    hana::Sequence) to conceptify his API, I encourage him to do so. This
may
    make it possible to use any tuple-like sequence for the children of an
    expression, thereby reducing the coupling between the two libraries. If
    Zach wants to explore using Fusion instead of Hana, that is also an
option
    that would make MSVC support achievable.

- A single macro for symmetric binary operators
    It was brought up that there's no macro to define both `arg <op> expr`
    and `expr <op> arg` at once. I believe this was initially done to avoid
    potential conflicts with the member variants, which already provide
    `expr <op> arg`. I believe it should still be possible to do it, since,
    as Steven said, most of the time a user will need symmetric behavior
    anyway.

- External dependencies checked-into the repository
    The library includes internal copies of some of its dependencies, which
    is unusual for a Boost library. Zach said it wasn't his intent to put
the
    library into Boost that way if it were accepted.

To see all the issues that were raised during the review, you can search
for tickets labeled with `Boost Review` on GitHub:

    https://github.com/tzlaine/yap/issues?q=label%3A%22Boost+Review%22

Based on the concerns above, the following lists the conditions for
acceptance:

- Documentation
    In my opinion, this was the biggest concern to be addressed. A lot of
    progress was made during and after the review, but I still think the
    documentation can be improved and made more complete. I especially
    believe that integrating more examples inside the "tutorial" (not just
    at the end) and introducing new concepts more gradually would help
readers
    that are not already expression template gurus. It is very difficult to
    set a clear action item for "improving the documentation". For this
reason,
    Brook Milligan and I will work with Zach to define what it means for the
    documentation to meet this acceptance condition. Brook was kind enough
to
    agree to this when I asked him in a private communication.

- A single macro for symmetric binary operators
    The library must provide a way to define symmetric binary operators in
    one go. Exactly how this can be achieved and whether we should be able
    to SFINAE-constrain the arguments is left to Zach's discretion.

- External dependencies
    The external dependencies must not be packaged with the library itself.
    Another mechanism should be used; I leave that mechanism unspecified,
    but it should be possible to test the library using the usual
Boost.Build
    workflow (e.g. an ExternalPackage download that would only work in CMake
    is not acceptable).

- Boost.Build
    Yap needs to support Boost.Build like all the other Boost libraries.

I will post an update to this list when the conditions of the review are
met.
If they wish to do so, reviewers can then verify that the conditions are met
in a satisfactory fashion and the library will automatically be accepted
unless someone objects.

I would like to thank everyone who participated in the review, and
congratulations to Zach for his library.

Regards,
Louis Dionne