closure_type_t in xfunction

[JohanMabille]

As stated in #1070, using const_closure_type to store operands in an xfunction may prevent a call of the move constructor and lead to a call of the copy constructor (resulting in performance issue in the best case, and compilation issues in the worst case).

Indeed the const_closure_type_t of an rvalue of type T is const T, thus an rvalue operand of a function will be stored as a const value, which is not movable by default. A few solutions can fix this problem:

1. Use closure_type_t instead of const_closure_type_t

In that case, the closure_type of an rvalue of type T is T, which is movable by default. The problem is that references to non-const object will not be stored as const references in the xfunction object:

xt::xarray<double> a, b;
auto f = a + b;

Here a and b will be stored as xt::array<double>&, allowing the xfunction object to potentially modify them, which is semantically wrong. If this code had to be written without universal references, it would use const references for the type of operand parameters, not mutable references (and that's the way pre-c++11 expression systems are written). It might not be a big deal for now since all the methods of xfunction are const, but this might change (and when that happens, I'm not convinced we'll still have this problem in mind). Besides this breaks the const-correctness from my point of view (the node of an expression tree is not supposed to change its operands).

2. Explicitly write the move constructor / assign operator

Another solution is to explicitly write the move constructor / assign operator to const cast the operands stored as const T and then call their move constructor. This, again, breaks the const-correctness. Indeed, when an object is moved, its final state is not guaranteed to be valid, which is inconsistent with a const object.
However, since an operand stored as a value (const or mutable) is owned by the xfunction, it does not really matter, a change on this operand has no side effect (contrary to the change on a reference on an expression that might be used elsewhere); besides, storing value operands as const T instead of T ensures that we always call the const overload of the operand's methods. As for previous solution, it is not a big deal for now since the methods of xfunction are all const (ensuring the call of the const overloads of the operand's methods), but this might change.

3. Use another type of closure type

If we don't have to ensure the call of const methods on the operands of a function, another (simpler) solution is to store lvalue operands as const references and rvalue operands as mutable values. This ensures the xfunction and its operands can always be moved, and that no side effect arises. This solution requires a new kind of closure type (always const for reference type, never const for value type).

Indeed we don't want to change the implementation of const_closure_type because this is a more general concept than its usage in xtensor, and one could need the guarantee to call the const overloads as explained in 2).

I think the best solution for xtensor is the last one:

• we don't need to ensure a call of const overloads on operands
• we want to avoid potential side effects of xfunction on its operands (no change allowed on lvalue operands, a potential change on an rvalue operand (i.e. owned by the xfunction) doesn't matter).

NB: Note that this issue is not restricted to xfunction, a lot of expressions in xtensor indirectly use const_closure_type through const_xclosure.

[wolfv]

There might be another option: store a const arg tuple and implement a move constructor in xfunction.

Afaik when I tested std::get in a const tuple it returned a const value

[JohanMabille]

The constness of the tuple is an implementation detail while the constness of constructor arguments (which explicitly states that no side effect occurs) is a specification of the API.

Besides, this is specific to xfunction and does not solve the problem for other kinds of expressions.

[SylvainCorlay]

I think that solution (1) is the simplest solution. Almost nothing to do :)

I don't think that it breaks const correctness.

[JohanMabille]

"almost nothing to do" is not a technical argument ;)

What can be const should be const. Would you accept something like this?

template <class E>
struct unary_operator
{
    unary_operator(E& operand);
}

Not me, I want the guarantee that the expression node won't change its operand, otherwise it's not const correct.

[SylvainCorlay]

Well, simple is better than complex, complex better than complicated.

The const closure issue is actually a good argument that we may not want to constify everything all the time...

[JohanMabille]

Well, simple is better than complex, complex better than complicated.

But here "simple" breaks the principle of design "What can be const should be const". Besides I don't think that adding a new closure type in xtl is complicated, this is a backward compatible change and the change is xtensor is pretty straightforward.

The const closure issue is actually a good argument that we may not want to constify everything all the time...

Indeed, we don't want to constify the members stored as values, for those stored as reference we want to use const as mush as possible to explicitly state that no side effect can occur. Again, I'm pretty sure you wouldn't accept the code I exposed above, which is morally equivalent to removing the const_closure in xfunction.

EDIT: rewording because the first version was a bit rude, which was not my intention.

[wolfv]

just to make this even a bit more urgent: one can see the copy instructions in the assembly for xscalars! :)

[wolfv]

fixed in latest xtl release, test added to xtensor.