Home Original page

adding xt::detail::has_fixed_size and replacing is_array to expand supported types. by vakokako · Pull Request #2556 · xtensor-stack/xtensor

Conversation

@vakokako

This pr adds type trait xt::detail::has_fixed_size to replace a common usage of xt::detail::is_array where implementation can benefit if the size of the container (usually used for shape) is known at compile time. This allows to add xt::sequence_view for example to supported types for shape in xt::adapt, but even much much more.

This also adds xt::detail::get_fixed_shape as a wrapper to std::tuple_size. The reason this is needed is that for example for xfunction that has fixed shape, we also want to use std::tuple_size, but we cannot partially specialize std::tuple_size only for the cases when xfunction has fixed shape.

auto tensor = xt::eval(xt::zeros<double>(xt::adapt(std::array<size_t, 2>{}) / 2));
// before decltype(tensor) == xarray
// after decltype(tensor) == xtensor

@vakokako

The build only fails on the older msvc 19.0, but it's not clear why. It says use of undefined type 'xt::detail::get_fixed_size<T,void>', but this should use the specialization in xshape.hpp at line 399.

JohanMabille

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error on Windows seems legit.

# pragma clang diagnostic ignored "-Wmismatched-tags"
#endif

namespace std

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good practice is to avoid opening std namespace. You can directly specialize tuple_size with the following syntax:

tempate <class ET, class S, xt::layout_type L, bool SH, class TAG>
struct std::tuple_size<xt::xfixed_container<ET, S, L, SH, Tag>> : 
// ...

Also pay attention that tuple_size is a struct, not a class (therefore, no need to specify a public inheritance, and this will avoid a lot of warnings).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specialization was done in the same style as here:

class tuple_size<xt::const_array<T, N>> :

It also contains a comment about tuple_size being a class in c++ standard, I can just copy the comment here, or update also the xstorage version.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial comment was based on the standard definition of tuple_size. However, it appears that th standard uses class tuple_size in other parts of the spec, and is therefore inconsistent. libc++ is consistent and uses class everywhere, so we can keep it. See this discussion for more detail about this issue.

The specializations in xstorage.hpp should be updated to avoid opening the std namespace, though.

2 participants

@vakokako @JohanMabille