[Boost-announce] [boost] [Boost.Yap] Review results

Subject: [Boost-announce] [boost] [Boost.Yap] Review results
From: Louis Dionne via Boost (boost_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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost