Add constraint for layout_left|right|stride::stride(), and add test
#238
Conversation
There was a problem hiding this comment.
I don't really like introducing template parameters there. How about we just implement the constraint for C++20 and above, and then have the if constexpr condition and the specific test for availability protected also for C++20. Then note in the README that this constraint is not implemented in C++17/14.
| for(size_t r=0; r != this->exts1.rank(); ++r) { | ||
| ASSERT_EQ(map1.extents().extent(r), map2.extents().extent(r)); | ||
| ASSERT_EQ(map1.stride(r), map2.stride(r)); | ||
| if constexpr (TestFixture::exts_1_t::rank() > 0) { |
There was a problem hiding this comment.
Given my other comment, I would just protect the condition with whether C++20 is on.
There was a problem hiding this comment.
Works for me. Will update.
There was a problem hiding this comment.
Updated. May not be the best-looking #ifs...
There was a problem hiding this comment.
This code should be fine even if MDSPAN_HAS_CXX_20 defined, because the constraint on stride(r) won't come into play, no? I'd rather have fewer macros if we can avoid it.
There was a problem hiding this comment.
This check is for the case when exts_1_t::rank() == 0 in the C++20 mode, in which case we have the constraint and it'll result in a compilation error without the if constexpr.
Well, that being said, I think we should only protect the line that calls stride(). I'll fix that.
| @@ -176,6 +176,12 @@ class layout_left::mapping { | |||
| MDSPAN_INLINE_FUNCTION constexpr bool is_exhaustive() const noexcept { return true; } | |||
| MDSPAN_INLINE_FUNCTION constexpr bool is_strided() const noexcept { return true; } | |||
|
|
|||
| MDSPAN_TEMPLATE_REQUIRES( | |||
There was a problem hiding this comment.
I don't like introducing a template parameter here for C++17/14. I would say we just do this only for C++20 using a requires clause on a non-templated function, and note down in the readme that this constraint is not implemented pre-c++20,
tests/test_mdarray_ctors.cpp
Outdated
| @@ -143,15 +143,21 @@ void check_correctness(MDA& m, size_t rank, size_t rank_dynamic, | |||
| ASSERT_EQ(m.rank_dynamic(), rank_dynamic); | |||
| if(rank>0) { | |||
| ASSERT_EQ(m.extent(0), extent_0); | |||
| ASSERT_EQ(m.stride(0), stride_0); | |||
| if constexpr (MDA::extents_type::rank()>0) { | |||
| for(size_t r=0; r != this->exts1.rank(); ++r) { | ||
| ASSERT_EQ(map1.extents().extent(r), map2.extents().extent(r)); | ||
| ASSERT_EQ(map1.stride(r), map2.stride(r)); | ||
| if constexpr (TestFixture::exts_1_t::rank() > 0) { |
There was a problem hiding this comment.
This code should be fine even if MDSPAN_HAS_CXX_20 defined, because the constraint on stride(r) won't come into play, no? I'd rather have fewer macros if we can avoid it.
| @@ -143,15 +143,33 @@ void check_correctness(MDA& m, size_t rank, size_t rank_dynamic, | |||
| ASSERT_EQ(m.rank_dynamic(), rank_dynamic); | |||
| if(rank>0) { | |||
| ASSERT_EQ(m.extent(0), extent_0); | |||
| ASSERT_EQ(m.stride(0), stride_0); | |||
| #if MDSPAN_HAS_CXX_20 | |||
There was a problem hiding this comment.
can't we just move that above 144. After all runtime wise these checks anyway only execute if rank>0 and we don't need to check that it compiles, there are other tests which do taht.
There was a problem hiding this comment.
And I mean with that we only need one big constexpr if protection for all runtime check rank clauses.
There was a problem hiding this comment.
That makes sense. Updated.
Adopt post-mdspan-0.3.0 and implement P1673R9 fully
Addresses #201.
Had to update a few existing tests to get them to pass.