Fix return type of templated span.subspan()#625
Fix return type of templated span.subspan()#625neilmacintosh merged 7 commits intomicrosoft:masterfrom
Conversation
include/gsl/span
Outdated
| #if _MSC_VER < 1910 | ||
| #pragma push_macro("constexpr") | ||
| #define constexpr /*constexpr*/ | ||
| #define USE_STATIC_CONSTEXPR_WORKAROUND |
There was a problem hiding this comment.
GSL_USE_STATIC_CONSTEXPR_WORKAROUND
There was a problem hiding this comment.
Oops! Thanks.
|
|
||
| constexpr static const index_type extent = Extent; | ||
|
|
||
| #if defined(GSL_USE_STATIC_CONSTEXPR_WORKAROUND) |
There was a problem hiding this comment.
This is an odd choice. Why not simply provide the out-of-class definition for C++ < 17?
There was a problem hiding this comment.
From memory: one of the compilers gave me a complaint about it. Plus, this seemed like the very well-known-but-ugly moral equivalent of the C++17 constexpr (i.e. it has a very good chance of actually getting optimized away). It's only a temporary band-aid, so I'm not overly attached, but nor did I want to spend too much time perfecting it.
There was a problem hiding this comment.
Fair enough. I'd suggest fixing the underlying type of the enum so extent always has the same type:
enum : index_type { extent = Extent };There was a problem hiding this comment.
Yep, sounds good!
include/gsl/span
Outdated
| template <class ElementType, std::ptrdiff_t Extent, std::ptrdiff_t Offset, std::ptrdiff_t Count> | ||
| struct calculate_subspan_type | ||
| { | ||
| using type = span<ElementType, Count != dynamic_extent ? Count : (Extent != dynamic_extent ? Extent - Offset - 1 : Extent)>; |
There was a problem hiding this comment.
Extent - Offset - 1 means that given:
int data[5]{};
span<int, 5> s = data;decltype(s.subspan<5>()) is span<int, -1> instead of span<int, 0>, and decltype(s.subspan<0>()) is span<int, 4> instead of span<int, 5>. Surely it should be an invariant that s.subspan<0>() == s for a span s. This expression should be Extent - Offset.
There was a problem hiding this comment.
There was a problem hiding this comment.
You are completely right. Nice catch. That will teach me for working at night.
There was a problem hiding this comment.
In passing...I took a look at the ranges implementation and it doesn't seem to match the intended design for the templated subspan: that people can pass a dynamic_extent value for the second template parameter to mean "consume everything that remains". (symmetrical to the way the non-template subspan works). Is that something you guys chose to do explicitly, or just an outcome of the implementation choice?
There was a problem hiding this comment.
Is that something you guys chose to do explicitly, or just an outcome of the implementation choice?
That is a bug I introduced when I split subspan out into multiple members. It didn't occur to me that someone would explicitly specify dynamic_extent for the second template parameter instead of simply omitting it. It apparently didn't occur to you either, there's no test coverage of such a call to subspan. ;)
EDIT: Filed as ericniebler/range-v3#799.
There was a problem hiding this comment.
Oh, and thanks for the bug report!
There was a problem hiding this comment.
Hahahaha. I thought about the lack of test coverage just after posting the comment....I had a distinct memory you'd run these tests against the implementation...so it's all really my fault (spoiler alert: most things are).
There was a problem hiding this comment.
And thanks for all the help! You came for the review, stayed for the bug report!
tests/span_tests.cpp
Outdated
| { | ||
| span<int, 5> av = arr; | ||
| CHECK((av.subspan<1>().size() == 3)); | ||
| CHECK(decltype(av.subspan<1>())::extent == 3); |
There was a problem hiding this comment.
Ditto for these two lines: decltype(av.subspan<1>())::extent should be 4.
There was a problem hiding this comment.
Good point! My brain clearly had extent and size confused.
2 participants
This PR modifies the return type of the templated version of
span::subspan()so that it returns a fixed-sized span wherever possible. This is important for encouraging optimization of code that usessubspan().Writing the tests for this made me actually use
span::extentwhich made me realize that it does not work unless you are using a C++17-compliant compiler, so I also added a workaround to makespan::extentavailable on C++11/14 compilers until we can all live in the better world of C++17. ;-)