-
Notifications
You must be signed in to change notification settings - Fork 764
Make gsl::span's iterators use the contiguous_iterator concept #1035
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make gsl::span's iterators use the contiguous_iterator concept #1035
Conversation
sunnychatterjee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the fix Dmitry!
include/gsl/span
Outdated
| class span_iterator | ||
| { | ||
| public: | ||
| #ifdef __cpp_lib_concepts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use __cpp_lib_ranges to detect support for std::contiguous_iterator_tag, __cpp_lib_concepts only indicates that the contents of <concepts> are available. Note that we have no portable guarantee that __cpp_lib_ranges is defined unless we've included the C++20 header <ranges> or <version>; I suggest conditionally including <version> on line 29ish:
#ifdef __has_include
#if __has_include(<version>)
#include <version>
#endif
#endifand changing this line (and line 115) to instead refer to __cpp_lib_ranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In STL span, their usage of std::contiguous_iterator_tag is guarded by __cpp_lib_concepts
template <class _Ty>
struct _Span_iterator {
#ifdef __cpp_lib_concepts
using iterator_concept = contiguous_iterator_tag;
#endif // __cpp_lib_conceptsIn VS 2019 it looks like __cpp_lib_ranges is guarded by CXX23, in yvals_core.h, and this condition is not met. So the changes in this PR do not compile in VS2019.
#if _HAS_CXX23 && defined(__cpp_lib_concepts) // TRANSITION, GH-395 and GH-1814
#define __cpp_lib_ranges 201911L
#endif // _HAS_CXX23 && defined(__cpp_lib_concepts)I see that your STL PR microsoft/STL#2518 addresses that, but those changes are only available in VS2022.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to support VS 2019 before we backport the final C++20 changes (which will be another month or two) you should still have the portable test for __cpp_lib_ranges but also specifically detect MSVCSTL in "close enough" mode:
#if defined(__cpp_lib_ranges) || (defined(_MSVC_STL_VERSION) && defined(__cpp_lib_concepts))
tests/span_tests.cpp
Outdated
| gsl::span<int> gsl_span; | ||
| std::span<int> std_span = gsl_span; | ||
| (void)std_span; // suppress unused variable warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend:
| gsl::span<int> gsl_span; | |
| std::span<int> std_span = gsl_span; | |
| (void)std_span; // suppress unused variable warning | |
| int arr[5] = {1, 2, 3, 4, 5}; | |
| gsl::span<int> gsl_span{arr}; | |
| #ifdef __cpp_lib_ranges | |
| EXPECT_TRUE(std::to_address(gsl_span.begin()) == gsl_span.data()); | |
| EXPECT_TRUE(std::to_address(gsl_span.end()) == gsl_span.data() + gsl_span.size()); | |
| #endif // __cpp_lib_ranges | |
| std::span<int> std_span = gsl_span; | |
| EXPECT_TRUE(std_span.data() == gsl_span.data()); | |
| EXPECT_TRUE(std_span.size() == gsl_span.size()); |
which will fail at EXPECT_TRUE(std::to_address(gsl_span.end()) == gsl_span.data() + gsl_span.size()); since operator-> isn't well-defined for the end iterator of a span. (contiguous_iterator requires std::to_address(meow) to work, which defaults to returning operator->().)
I assume you don't want operator-> to work for past-the-end iterators any more than we do, so you'll need to specialize std::pointer_traits and implement to_address to tell std::to_address how to convert your iterators into pointers.
Actually, it would be easier to make a PR into your branch than type this up. I've submitted dmitrykobets-msft#1.
|
The VS 2019 clang-cl tests are failing because that old version of clang doesn't recognize the reserved-identifier warning suppression. You'll need to either figure out how to avoid using it with older clangs, or add |
The only missing part to satisfy this concept was to use the contiguous_iterator_tag
_Unwrapped is only available for MSVC. Instead, make the pointer_traits class a friend
This reverts commit ddd5b12. Apparently trying to suppress clang warnings is hard to do inline in the library headers. Instead, will simply suppress in the cmake config
3e09e4a to
4e2b905
Compare
Resolves #1016
The only missing part to satisfy the contiguous_iterator concept was to use the
contiguous_iterator_tag.Used c++ - In C++20, how do I write a contiguous iterator? - Stack Overflow as a reference.