Eye function

[wolfv]

Hi,

I've implemented the eye function through xgenerator. This time I tried to use a generic std::function that works on iterators.

However, I faced a difficulty when using the static array + std::begin/end() combination @SylvainCorlay demonstrated in the last PR. I didn't find any way to keep the std::function as a function template.

Do you have ideas / is it necessary?

I think if this works well, the fn_impl could get some nice helper-wrapper to initialize a xgenerator from a lambda function, or the mechanism could be integrated directly into xgenerator.

[JohanMabille]

Sorry for responding so late, I was very busy these last days. I think we should avoid using std::function in xgenerator, except if we need to bind some arguments (and even in that case we should be able to store the result of a call to std::bind rather than a std::function). The reason is std::function adds a call to a virtual function; resolving such a call each time we access an element of xgenerator may hit performances drastically if the function we store is simple.

Besides, you can't "fix" the iterator's type to be used since the function can be called from operator() which internally uses pointers as iterators and from operator[] which internally uses std::vector iterators.

So it's nice you replaced the std::function with a functor structure since you solved two problems with a single change :)

I don't understand the compilation error on Windows yet, but I'm on it.

[wolfv]

Good to hear! :)

[JohanMabille]

It seems xtensor somehow breaks operator+ and operator- of std::vector iterators. That's something we already encountered. The workaround is to use a temporary iterator and operator+= / operator-= until we fix it (currently we have no clue why it's broken).

Another remark would be to make eye_fn::operator() a template method, so the structure doesn't depend on the iterator type. This way you can make Fa simple parameter in fn_impl template parameters list, and you can store the functor instead of instantiating it every time you need it.

[JohanMabille]

About the Windows compilation issue: The problem comes from get_xfunction_type in xoperation.hpp:

template <template <class...> class F, class... E>
        template <template <class...> class F, class... E>
        using get_xfunction_type = std::enable_if_t<has_xexpression<E...>::value,
                                                    xfunction<F<common_value_type<E...>>,
                                                                common_value_type<E...>,
                                                                get_xexpression_type<E>...>>;

The problem is Visual Studio evaluates the second argument of std::enable_if_t even if the first argument is false. Fixing this involves a lot of other workarounds due to VS bugs when mixing variadic template and template aliases; since it breaks operator+ and operator- for iterators inside the xt namespace only, and since we have a workaround, it's not something I'm going to fix for the moment.

[JohanMabille]

I don't understand why you use a third template parameter with a default value set to K<T>. It seems you don't use the K parameter elsewhere in this class, so maybe you could just do something like:

template <class T, template <class> class F>
class fn_impl
{
    using functor_type = F<T>;
    ....
};

Or, even better, add a value_type typedef in eye_fn, and retrieve it in fn_impl. This way you don't need the T template parameter.

[JohanMabille]

The best solution would be to deduce the value type from any callable type (including lambda), but that requires more work. This can be done after this feature has been integrated, though. Let me know which option you prefer.

[wolfv]

I changed things a bit more to accommodate for the k parameter. Let me know what you think.

[JohanMabille]

Perfect !