robotology / idyntree

Multibody Dynamics Library designed for Free Floating Robots
BSD 3-Clause "New" or "Revised" License
177 stars 67 forks source link

Make iDynTree::Span compliant with the std::span implementation #758

Closed GiulioRomualdi closed 4 years ago

GiulioRomualdi commented 4 years ago

std::span has been introduced in C++20. Since iDynTree is C++11 compliant a custom implementation of span is required. DIscussing with @traversaro I noticed that our current implementation contains methods that are not defined by the standard, e.g.

https://github.com/robotology/idyntree/blob/a97aa8407359b12cccc95313b00c654a0d444508/src/core/include/iDynTree/Core/Span.h#L507-L508

https://github.com/robotology/idyntree/blob/a97aa8407359b12cccc95313b00c654a0d444508/src/core/include/iDynTree/Core/Span.h#L515-L516

https://github.com/robotology/idyntree/blob/a97aa8407359b12cccc95313b00c654a0d444508/src/core/include/iDynTree/Core/Span.h#L499-L500

Furthermore, we introduced also introduced some custom constructors. Please check here the std::span constructors. e.g.

https://github.com/robotology/idyntree/blob/a97aa8407359b12cccc95313b00c654a0d444508/src/core/include/iDynTree/Core/Span.h#L402-L420

As far as I understood this constructor should be substituted with something like

template< class R >
explicit(extent != std::dynamic_extent)
constexpr span( R&& r );

indeed the span can be constructed using the ranges library. https://en.cppreference.com/w/cpp/ranges. Unfortunately the ranges library has been introduced with C++20. So we cannot use it as it is.

The above constructors are not compatible with Eigen objects.
Indeed

Eigen::Vector3d v;
iDynTree::Span <double> s(v)

will not compile because Eigen::Dense does not defined Container::pointer typedef.

On the other hand, Eigen 3.4 will provide STL compatible iterators. Please check here. So I hope it will also be compatible with std::ranges. (Probably we should double-check on this)

If Eigen will support std::ranges the following code will compile

#include <span>
#include <Eigen/Dense>

int main ()
{
    Eigen::Vector3d m;
    std::span<double> s(m);

    return 0;
}
Click here to check the error

``` [ 50%] Building CXX object CMakeFiles/eigen_test.dir/span_eigen.cpp.o /home/gromualdi/robot-code/test_eigen/span_eigen.cpp: In function ‘int main()’: /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:9:26: error: no matching function for call to ‘std::span::span(Eigen::Vector3d&)’ 9 | std::span s(m); | ^ In file included from /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:2: /usr/include/c++/10/span:234:2: note: candidate: ‘template requires (_Extent == std::dynamic_extent || _OExtent == std::dynamic_extent || _Extent == _OExtent) && (std::__is_array_convertible<_Type, _Tp>::value) constexpr std::span<_Type, _Extent>::span(const std::span<_OType, _OExtent>&) [with _OType = _OType; long unsigned int _OExtent = _OExtent; _Type = double; long unsigned int _Extent = 18446744073709551615]’ 234 | span(const span<_OType, _OExtent>& __s) noexcept | ^~~~ /usr/include/c++/10/span:234:2: note: template argument deduction/substitution failed: /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:9:26: note: ‘Eigen::Vector3d’ {aka ‘Eigen::Matrix’} is not derived from ‘const std::span<_Tp, _Num>’ 9 | std::span s(m); | ^ In file included from /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:2: /usr/include/c++/10/span:226:7: note: candidate: ‘constexpr std::span<_Type, _Extent>::span(const std::span<_Type, _Extent>&) [with _Type = double; long unsigned int _Extent = 18446744073709551615]’ 226 | span(const span&) noexcept = default; | ^~~~ /usr/include/c++/10/span:226:12: note: no known conversion for argument 1 from ‘Eigen::Vector3d’ {aka ‘Eigen::Matrix’} to ‘const std::span&’ 226 | span(const span&) noexcept = default; | ^~~~~~~~~~~ /usr/include/c++/10/span:214:2: note: candidate: ‘constexpr std::span<_Type, _Extent>::span(_Range&&) [with _Range = Eigen::Matrix&; _Type = double; long unsigned int _Extent = 18446744073709551615]’ 214 | span(_Range&& __range) | ^~~~ /usr/include/c++/10/span:214:2: note: constraints not satisfied In file included from /usr/include/c++/10/array:41, from /usr/include/c++/10/span:42, from /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:2: /usr/include/c++/10/bits/range_access.h: In instantiation of ‘constexpr std::span<_Type, _Extent>::span(_Range&&) [with _Range = Eigen::Matrix&; _Type = double; long unsigned int _Extent = 18446744073709551615]’: /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:9:26: required from here /usr/include/c++/10/bits/range_access.h:862:13: required for the satisfaction of ‘range<_Tp>’ [with _Tp = Eigen::Matrix&] /usr/include/c++/10/bits/range_access.h:909:13: required for the satisfaction of ‘input_range<_Tp>’ [with _Tp = Eigen::Matrix&] /usr/include/c++/10/bits/range_access.h:913:13: required for the satisfaction of ‘forward_range<_Tp>’ [with _Tp = Eigen::Matrix&] /usr/include/c++/10/bits/range_access.h:918:13: required for the satisfaction of ‘bidirectional_range<_Tp>’ [with _Tp = Eigen::Matrix&] /usr/include/c++/10/bits/range_access.h:923:13: required for the satisfaction of ‘random_access_range<_Tp>’ [with _Tp = Eigen::Matrix&] /usr/include/c++/10/bits/range_access.h:928:13: required for the satisfaction of ‘contiguous_range<_Range>’ [with _Range = Eigen::Matrix&] /usr/include/c++/10/bits/range_access.h:862:21: in requirements with ‘_Tp& __t’ [with _Tp = Eigen::Matrix&] /usr/include/c++/10/bits/range_access.h:864:15: note: the required expression ‘std::ranges::__cust::begin(__t)’ is invalid 864 | ranges::begin(__t); | ~~~~~~~~~~~~~^~~~~ /usr/include/c++/10/bits/range_access.h:865:13: note: the required expression ‘std::ranges::__cust::end(__t)’ is invalid 865 | ranges::end(__t); | ~~~~~~~~~~~^~~~~ cc1plus: note: set ‘-fconcepts-diagnostics-depth=’ to at least 2 for more detail In file included from /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:2: /usr/include/c++/10/span:202:2: note: candidate: ‘template requires std::__is_compatible_array::value constexpr std::span<_Type, _Extent>::span(const std::array<_Tp, _ArrayExtent>&) [with _Tp = _Tp; long unsigned int _ArrayExtent = _ArrayExtent; _Type = double; long unsigned int _Extent = 18446744073709551615]’ 202 | span(const array<_Tp, _ArrayExtent>& __arr) noexcept | ^~~~ /usr/include/c++/10/span:202:2: note: template argument deduction/substitution failed: /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:9:26: note: ‘Eigen::Vector3d’ {aka ‘Eigen::Matrix’} is not derived from ‘const std::array<_Tp, _Nm>’ 9 | std::span s(m); | ^ In file included from /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:2: /usr/include/c++/10/span:195:2: note: candidate: ‘template requires std::__is_compatible_array<_Tp, _ArrayExtent>::value constexpr std::span<_Type, _Extent>::span(std::array<_Tp, _ArrayExtent>&) [with _Tp = _Tp; long unsigned int _ArrayExtent = _ArrayExtent; _Type = double; long unsigned int _Extent = 18446744073709551615]’ 195 | span(array<_Tp, _ArrayExtent>& __arr) noexcept | ^~~~ /usr/include/c++/10/span:195:2: note: template argument deduction/substitution failed: /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:9:26: note: ‘Eigen::Vector3d’ {aka ‘Eigen::Matrix’} is not derived from ‘std::array<_Tp, _Nm>’ 9 | std::span s(m); | ^ In file included from /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:2: /usr/include/c++/10/span:188:2: note: candidate: ‘template requires _Extent == std::dynamic_extent || _ArrayExtent == _Extent constexpr std::span<_Type, _Extent>::span(std::type_identity_t<_Type> (&)[_ArrayExtent]) [with long unsigned int _ArrayExtent = _ArrayExtent; _Type = double; long unsigned int _Extent = 18446744073709551615]’ 188 | span(type_identity_t (&__arr)[_ArrayExtent]) noexcept | ^~~~ /usr/include/c++/10/span:188:2: note: template argument deduction/substitution failed: /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:9:26: note: mismatched types ‘std::type_identity_t [_ArrayExtent]’ and ‘Eigen::Vector3d’ {aka ‘Eigen::Matrix’} 9 | std::span s(m); | ^ In file included from /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:2: /usr/include/c++/10/span:174:2: note: candidate: ‘template requires (contiguous_iterator<_It>) && (sized_sentinel_for<_End, _It>) && ((std::__is_compatible_ref)())>::value) && !(is_convertible_v<_End, std::span<_Type, _Extent>::size_type>)) constexpr std::span<_Type, _Extent>::span(_It, _End) [with _It = _It; _End = _End; _Type = double; long unsigned int _Extent = 18446744073709551615]’ 174 | span(_It __first, _End __last) | ^~~~ /usr/include/c++/10/span:174:2: note: template argument deduction/substitution failed: /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:9:26: note: candidate expects 2 arguments, 1 provided 9 | std::span s(m); | ^ In file included from /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:2: /usr/include/c++/10/span:160:2: note: candidate: ‘template requires (contiguous_iterator<_It>) && (std::__is_compatible_ref)())>::value) constexpr std::span<_Type, _Extent>::span(_It, std::span<_Type, _Extent>::size_type) [with _It = _It; _Type = double; long unsigned int _Extent = 18446744073709551615]’ 160 | span(_It __first, size_type __count) | ^~~~ /usr/include/c++/10/span:160:2: note: template argument deduction/substitution failed: /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:9:26: note: candidate expects 2 arguments, 1 provided 9 | std::span s(m); | ^ In file included from /home/gromualdi/robot-code/test_eigen/span_eigen.cpp:2: /usr/include/c++/10/span:152:7: note: candidate: ‘constexpr std::span<_Type, _Extent>::span() requires _Extent + 1 <= 1 [with _Type = double; long unsigned int _Extent = 18446744073709551615]’ 152 | span() noexcept | ^~~~ /usr/include/c++/10/span:152:7: note: candidate expects 0 arguments, 1 provided ```

So after double-checking the future compatibility of Eigen with std::span we can think to modified the constructors in order to work also with Eigen objects.

cc @traversaro @S-Dafarra and @francesco-romano

S-Dafarra commented 4 years ago

Rather than modifying the constructor, IMHO it would be better to define custom make_span functions (that are not present in the standard either), that can remain even after moving to the standard span. These functions may check if the data and size methods are defined in a SFINAE fashion. I did something similar in bipedal-locomotion-framework. For example, in this static_assert I check if span is constructible given the inputs, or if the data and size methods are defined. According to the case, I construct the span in different ways.

traversaro commented 4 years ago

Any solution is fine for me, even because I think in any case we will never do:

namespace iDynTree 
{ 
    typedef std::span Span;
}

so even not having 100% std::span compatibility is ok.

traversaro commented 4 years ago

Using Eigen with Span constructor should be solved by https://github.com/robotology/idyntree/pull/766, but I am not sure if this issue was covering also something else.

GiulioRomualdi commented 4 years ago

In theory yes