For each issue raised during the review, please create an issue here and tag it with "Boost Review". Then, please include a link to the corresponding GitHub issue when replying on the Boost mailing list. This makes it much easier to keep track of everything and provides clear action items for everyone.

Be really, really clear in the docs that making a terminal that contains a value requires std::move().

Based on this:

I think something like this should be usable in most cases:

// Let's assume the existence of a my_make_term() function that takes a Yap terminal
// and a Context &, and returns a new Yap terminal.
template
struct transform_terminals_with_context
{
// Base case. Note that I'm ignoring placeholders entirely for this
// example (they're easy to special-case if necessary).
template
auto operator() (yap::terminal_tag, T && t)
{ return my_make_term(std::forward(t), context_); }

// Recursive case: Match any binary expression.
template <typename Tag, typename LExpr, typename RExpr>
auto operator() (Tag, LExpr const & left, RExpr const & right)
{
    return yap::make_expression<yap::detail::kind_for<Tag>>(
        boost::yap::transform(left, *this),
        boost::yap::transform(right, *this));
}

// Recursive case: Match any unary expression.
template <typename Tag, typename Expr>
auto operator() (Tag, Expr const & expr)
{
    return yap::make_expression<yap::detail::kind_for<Tag>>(
        boost::yap::transform(expr, *this));
}

// Ternary and call are added as necessary.

Context & context_;

};

This transforms all your terminals to new terminals, using your custom code that applies your terminal + context operation. You can then just eval() the transformed expression using the default evaluate(), transform() it again with a different transform, etc. I had to make up the constexpr function kind_for<>() that maps expression-kind tag types to yap::expr_kind values (which I'll probably add now that I see it is needed TODO), but the reset is just typical Yapery.

From Brook Milligan:

• A simple Expression concept easily modeled by user code. Member and non-member functions on Expressions can be added with compact macros, and a reference implementation exists for prototyping or experimentation.

• Evaluation of expression trees matches the semantics of builtin C++ expressions as closely as possible. All operator expressions are evaluated using the corresponding operator definitions for the types involved. This leads to clearer understanding of the semantics of expression tree evaluation, because the definitions are local to the types involved. This is one point at which the design of Yap diverges significantly from that of Proto; in the latter, the semantics of expression tree evaluation is determined by context classes. Consequently, it is much more difficult to reason about the semantics in Proto than in Yap. [ This last bit could also be in a Proto v. Yap section. ]

• Expression trees may be transformed explicitly in a user-defined way. This is accomplished with overloaded member functions in a transform class, each one corresponding to subexpressions in the expression tree. While these member functions may transform a subexpression into anything, a common pattern is to transform terminals into either new terminals or appropriate values and to leave other subexpressions unchanged. This evaluate(transform()) idiom is expected to be one of the most common ways of using Yap to manipulate and evaluate expression trees. Even though this use case involves copying the expression tree, it is expected that the new terminals or values will be inexpensive to copy so that the cost of transformation is small.

• Functions that operate on expression trees or create expressions. A set of functions is provided (and used by the library) to manipulate expression trees or their components. These should simplify the process of, for example, writing user-defined transforms.

• Evaluation of expression trees is generally [ what does this mean? ] equivalent to evaluating the corresponding native code. Thus, there should be no [ true? ] performance cost to using expression trees.

• Expression trees may be encapsulated within function objects that evaluate the expression when called. This provides a nice bridge with other function-based libraries.

• ... [ Maybe there are other (or fewer) features to emphasize. The point is to expand a bit on what the features mean to the user rather than just list them. It is also to organize them linearly from most to least important/relevant to the user. ]

Via the Boost, list, Paul Bristow says:

RTFM ;-)

http://www.boost.org/doc/libs/1_63_0/tools/auto_index/doc/html/index.html

You can just get indexes of macro, class, functions, and by adding index terms to your .idx file, you can get a subject index too.

To see the autoindex in (big!) action, look at Boost.Math docs.

Attached example may help too

modular-boost\libs\circular_buffer\doc\jamfile.v2 may help is making the admittedly non-trivial changes to get a full index.

You need some items in your .qbk file too (to say which indexes you want).

[/Include the indexes (class, function and everything) ]
'''

<title>Class Index</title>

<index type="function_name">
 <title>Function Index</title>
</index>

'''

To resolve this:

expression.hpp:
11-13: \note Due to a limitation of Doxygen, each of the
value(), left(), right(), and
operator overloads listed here is a stand-in for three member
Did you try \overload? That sometimes helps.

Transforms as evaluations, transforms that have context-like state, etc.

gcov says the following are not executed by the tests
(line numbers as of master):

algorithm.hpp:
  130 (value_expr_impl),
  232, 246 (get_impl),
  300 (get_c),
  408 (callable),
  427 (argument)
  701 (most of op_string)

expression.hpp:
  54, 62 (value),
  69, 77 (left),
  84, 92 (right),
  most unary operators (except negate)
  most binary operators (except multiplies and plus)
  247 (call),
  310 (value),
  428-420 (pre_dec, post_inc, post_dec)
  most binary operators (except multiplies, plus, and minus)

operators.hpp:
  many unary and binary operators.

print.hpp
  42 (print_value) - Isn't this overload for
      placeholders, rather than hana::llong?
  50,52,54 (print_type),
  87,89 (print_impl)

detail/default_eval.hpp:

  20 (eval_placeholder)
  many unary and binary operators.

You could also get rid of the need for ::user_expr

We actually want that. It's often necessary to be able to return an
expression created from some template different than the template
defining
he operator.

If that's really important then why doesn't
BOOST_YAP_USER_FREE_BINARY_OPERATOR take two
expr_template parameters?

Because there's only one result. The expr_template parameter governs how
the result is constructed, not which inputs are accepted.

Doesn't this make BOOST_YAP_USER_FREE_BINARY_OPERATOR
unusable, since expression already uses it, so using it
again with a different ExpressionTemplate would be ambiguous?

Sure, if you use both of them. I expect people only to use expression for quick-and-dirty small-scale uses of Yap, or for experimentation though.

However, the same problem exists if I have ET1 and ET2 that want to use BOOST_YAP_USER_FREE_BINARY_OPERATOR. In that case, the same ambiguity exists. The solution is the same as any operator overload ambiguity, I think: sfinae. If there's not already an appropriate constraining macro to replace BOOST_YAP_USER_FREE_BINARY_OPERATOR (I'd have to look around a bit), I should add one. TODO

Would it be possible to evaluate (by default or as an option) to evaluate expression upon destruction.

For example, normally in expression template libraries, one has some sort of evaluate function,

terminal a = ...
terminal b = ...
terminal c = ...
evaluate(a = b + c);

In principle, destruction on ; could trigger the evaluation.

a = b + c;

I think it is possible to do because subexpression could be flagged not to evaluate upon their destruction.

Another example from hello_world:

#include <boost/yap/expression.hpp>
#include <iostream>
int main (){
//traditional:    
// evaluate(boost::yap::make_terminal(std::cout) << "Hello" << ',' << " world!\n");
// automatic evaluation on destruction of a temporary
    boost::yap::make_terminal(std::cout) << "Hello" << ',' << " world!\n";
// or at least:    
//    boost::yap::make_auto_terminal(std::cout) << "Hello" << ',' << " world!\n";
    return 0;
}

This way the syntax will be more natural.
What do you think?

The Hof should come in handy with this.

On Fri, Feb 16, 2018 at 4:26 PM, Peter Dimov via Boost [email protected] wrote:
Zach Laine wrote:
In this case, something like the expression_tag<> scheme Peter recommended helps with this particular ambiguity:

template <typename Tag, typename... T>
auto operator()(expression_tag, T &&... t)

What I suggested was rather

template <expr_kind Kind, typename... T>
auto operator()(expression_tag, T &&... t)

as this enforces the necessary 1:1 correspondence between kinds and tags.

Right. I missed that the first time. Thanks.


I am interested in how a small linear algebra library would be written in yap. Please take a look at the Daixtrose Docs page 7 (chapter 3.2). I expect Yap to provide a more elegant approach to this. How would you perform this task in Yap?

From pipable_algorithms.cpp:
// This is a bit loose, because it allows us to write "sort(v) |
// unique(u)" or similar. It works fine for this example, but in
// production code you may want to write out the three member functions
// generated by this macro, and add SFINAE or concepts constraints on what
// is accepted on the right.
BOOST_YAP_USER_BINARY_OPERATOR_MEMBER(bitwise_or, ::algorithm_expr)

We really should have another macro that has a place for a trait-based constraint here. Looking at the other macros in the full list in the docs, it seems like there's a lot of room for a lot more macros here.

Googling "C++ Yap" yields a bunch of other stuff. I think this is a really cool and important project, so I think it deserves a more memorable name.

vector_alloc_test.cpp:

• Your replacement of operator new is technically
    illegal, because it can return a null pointer.

• I don't think the standard guarantees that vector makes
    any specific number of allocations.  The correct
    way is to set allocations = 0 after initializing
    the vectors and then verify that no further allocations
    happen.  Alternately, make operator new throw a bad_alloc
    during the region where you expect no allocations.
    As a bonus, it'll be easier to find the cause of
    unexpected allocations, when they do happen.

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.

CMake files within yap make use of variables such as ${CMAKE_SOURCE_DIR} which make it difficult to include yap as a sub-module of an external project. Use of ${CMAKE_CURRENT_SOURCE_DIR} would be more appropriate.

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.

It won't build with Clang 3.8 and GCC 6. It would be nice to document which minimum versions are required.

examples/vector.html:

• return boost::yap::make_terminal(std::move(vec[n]));
  Move is probably wrong as you're working with a reference
  to begin with. (Of course, it doesn't really matter for
  double, but imagine that you have a vector<unique_ptr>
  instead.)

The move is required to force a copy; just because the reference is valid
now doesn't mean it won't dangle later.

Sure, but when using the evaluate(transform()) idiom,
you're guaranteed that the references returned by transform
will not be left dangling.

Sure. But that's just in this example, and using that (admittedly
dominant) idiom. I still want to reinforce in the example code how you
make copies of value types, especially built-in ones.

Perfect-forwarding through a chain of
transforms is also an important use case that
needs to be explained. I don't mind having
the examples demonstrate copying as long as
they are written such in a way that copying
is really the correct behavior. In this particular
example, I don't think that it is.

That's a really good point. I'll revisit that example. TODO.

From Steven Watanabe:
detail/default_eval.hpp:
192,332: return eval_if_else(
It looks like this always evaluates both branches.

You need some compile-fail tests:

• invalid arguments to accessors, such as left, right, and get.
• possibly switching the arguments of transform.  (I find
    it mildly disturbing that this currently compiles, as
    transform is one function for which I may not always remember
    the correct argument order.)

From Steven Watanabe:
detail/default_eval.hpp:
149,302: BOOST_YAP_BINARY_OPERATOR_CASE(logical_or) // ||
150,303: BOOST_YAP_BINARY_OPERATOR_CASE(logical_and) // &&
No short circuit evaluation.

From Steven Watanabe:
detail/default_eval.hpp:
162,310: return eval_comma(
Please tell me that I'm reading this wrong and that
(a) this can safely return void, and (b) the left
argument is evaluated before the right argument.

I think you're forgetting that the terminal transform is
applied before calling operator()(xxx_tag, ...).
Thus, the result is actually:

make_expression<expr_kind::call>(checked_add{},
transform(transform(1_p, *this), *this),
transform(transform(2_p, *this), *this)).

Since the extra transform tacks on another call expr,
we end up up coming right back around to operator()(call_tag).

I was forgetting that! Thanks.

The bottom line is, any transform which is not idempotent
for terminals is badly broken if you unwrap terminals
automatically.

If you don't apply the terminal transform, but still
unwrap terminals, then it's still broken, because
then the terminal transform gets skipped entirely.

Actually, the code I posted fails to compile (even after
fixing the obvious typos) because the recursion makes
it impossible for auto to deduce the return type.

Right, and although this saves the user from runtime infinite recursion, it is a very obscure failure mode. This more than outweighs the convenience of the current interface. I'm convinced the auto-evaluating, and in fact all terminal unwrapping in tag-transforms, should be removed.

...

Actually, I take this back, at least partially -- I think auto-unwrapping
in some form might still be useful without auto-applying the transform.
I'll need to experiment with that a bit. TODO

If you can figure out a way to do it such that
unwrapping is only applied for arguments that are
explicitly intended to match terminals, then
it would work. If you imagine the semantics being
as if unwrapping the terminal were an implicit
conversion, then it would probably work correctly
in all cases. Unfortunately, I don't know how to
implement this.

I don't know if it needs to be so fancy. I could just revert back to the old behavior, which always unwraps terminals, but does not auto-apply transforms.

This should cover why things are done the way they are, and the overall mental model, in one place.

I'd guess that tag matching is a later addition, because it only works for simple cases. You can't do

   auto operator()(any_expression_tag, T&&... t);

I'm don't understand what that would mean, exactly.  What you can write though is:

template <typename Tag, typename... T>
auto operator()(Tag, T &&... t)

Is that different from the intended use case above?
 
[snip]

template<expr_kind kind, class... T>
auto get_arity::operator()(expression_tag, T const&... t) const
{
   return std::max( { yap::transform(t, get_arity{})... } );

}

I'm pretty sure you actually can write almost exactly this using the template I showed above.  I'll verify that this is the case.  That might make the example a lot easier to understand.

I keep forgetting to do this....

Links to evaluate() and evaluate_as() actually link to the transform() reference docs.

I used the following commands to build the project:
mkdir build
cd build
cmake ..
And get the following error while building the examples:
In file included from /home/tekne/Documents/Projects/yap/example/autodiff_library/autodiff.cpp:13:0: /home/tekne/Documents/Projects/yap/example/autodiff_library/autodiff.h:11:10: fatal error: boost/serialization/array_wrapper.hpp: No such file or directory #include <boost/serialization/array_wrapper.hpp>
Including and using the library itself produces no errors. Thanks!

221: template <long long I, typename Expr>
      decltype(auto) get (Expr && expr, hana::llong i);
    Does this need to be restricted to hana::llong, rather
    than, say, std::integral_constant?

It's the lack of nice syntax for integral_constant literals that made me
choose this.  I write get(expr, N_c) a lot, and I expect users to as
well.

   Supporting an IntegralConstant doesn't mean that
you can't pass 0_c.

Is the change from std::integral_constant to IntegralConstant significant?
That is, are you asking that I accept models of some concept, or just
std::integral_constant?

By "universal" I mean ones that cover the member and nonmember cases all in one macro. If possible, remove the non-universal ones (modulo those that are members-only, like a fancy eighties jacket).

deref.cpp:

• the test cases in this file only show pass/fail, they
    don't indicate the actual type returned by deref, which
    will make debugging failures harder.  I typically use
    BOOST_MPL_ASSERT((is_same<X, Y>));  specifically for
    this reason (even when static_assert is available).
• Actually, in general, these tests need to check both
    the type and the value.  Checking just the type is
    insufficient.

Specifically, tests are needed for transforming these expressions:

bare terminal (tag and expr matches)
terminal and nonterminal at the same time in same expression (tag and expr matches)

I've struggled trying to write what should have been a simple transformation: computing 'min_size' from an expression tree of lazy std::arrays. The following is my attempt using tag transforms.

#include <boost/yap/algorithm.hpp>
#include <boost/yap/print.hpp>

using namespace boost::yap;

template <boost::yap::expr_kind Kind, typename Tuple>
struct lazy_array_expr
{
    static const boost::yap::expr_kind kind = Kind;

    Tuple elements;

    BOOST_YAP_USER_BINARY_OPERATOR_MEMBER(plus, ::lazy_array_expr)
};

struct min_size
{
    template <size_t N>
    auto operator()(boost::yap::terminal_tag, std::array<int, N> const &)
    {
//        std::cout << "terminal tag, returning " << N << '\n';
        return make_terminal<lazy_array_expr, size_t>(std::move(N));
    }

    auto operator()(plus_tag, size_t N0, size_t N1)
    {
//        std::cout << "plus tag (" << N0 << ", " << N1 << ")\n";
        auto result = std::min(N0, N1);
        return make_terminal<lazy_array_expr, size_t>(std::move(result));
    }
};

int main()
{
    auto a1 = make_terminal<lazy_array_expr>(std::array<int, 1>{1});
    auto a2 = make_terminal<lazy_array_expr>(std::array<int, 2>{2, 2});
    auto a3 = make_terminal<lazy_array_expr>(std::array<int, 3>{3, 3, 3});

    auto t1 = transform(a1 + a2, min_size{});
    std::cout << "result: " << boost::yap::value(t1) << '\n'; /* this is computed correctly */

    auto t2 = transform(a1 + a2 + a3, min_size{});
    print(std::cout, t2); /* why is t2 is left in a state where further tag transforms could have been applied? */
    auto t3 = transform(t2, min_size{}); /* this should not have been necessary */
    std::cout << "result: " << boost::yap::value(t3) << '\n';
}

I find that the second application of this transform (in the sample code above as t2) fails to reduce the expression-tree into a single expression. The resulting t2 is:

expr<+>
    term<unsigned long>[=1]
    term<unsigned long>[=3]

which should have been further transformed by re-employing min_size::operator()(plus_tag, size_t N0, size_t N1)

This stands in contrast with the result of the first application (in the sample code as t1) on a smaller tree.

Perhaps the accompanying documentation should explain the process of tree traversal when applying a transformation.

I refer to this example:

template <boost::yap::expr_kind Kind, typename Tuple>
struct lazy_vector_expr
{
    static const boost::yap::expr_kind kind = Kind;

    Tuple elements;

    BOOST_YAP_USER_BINARY_OPERATOR_MEMBER(plus, ::lazy_vector_expr)
    BOOST_YAP_USER_BINARY_OPERATOR_MEMBER(minus, ::lazy_vector_expr)

    // Note that this does not return an expression; it is greedily evaluated.
    auto operator[] (std::size_t n) const;
};

I find it odd that lazy_vector_expr needs to be prepended with ::. Won't ADL grab the surrounding class anyway? If not: could you add an example for a class in a namespace?

Is there a technical reason for storing the value template argument in the field static const boost::yap::expr_kind of an expression?

IMHO this is redundant and could be replaced by a utility method that extracts the value from the template argument.