Skip to content
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

MSVC - gsl::make_span with iterators #982

Open
dingari opened this issue May 11, 2021 · 6 comments
Open

MSVC - gsl::make_span with iterators #982

dingari opened this issue May 11, 2021 · 6 comments
Assignees

Comments

@dingari
Copy link

dingari commented May 11, 2021

I have some code that uses gsl::make_span(cont.begin(), cont.end()) quite heavily. This compiles fine on macOS and Linux with GCC and Clang, but fails on MSVC. Instead, I have to use gsl::make_span(cont.data(), std::distance(cont.begin(), cont.end())) - which becomes pretty tedious and illegible.

Here's a quick example. Is there another overload we can use?

We can't pass the whole container to make_span, since we sometimes have varying end iterators returned from algorithms.

@JordanMaples
Copy link
Contributor

I'll take a look at it to see if there are any work arounds or fixes that we can make on the GSL side.

Would you mind creating an issue for this on the Developer Community? Please note in the issue that the code compiles properly with both Clang and GCC. Thanks

@beinhaerter
Copy link
Contributor

beinhaerter commented May 13, 2021

@JordanMaples Why should dingari open an issue on Developer Community? This issue is a GSL issue, not an STL issue and not a compiler issue.

A container's begin and end return iterators. There is no make_span and no span ctor taking two iterators. The nearest signature are make_span and span ctor taking two pointers. It looks like for std::array clang's and gcc's chose to let begin and end return a pointer while MSVC chose to return an iterator object.

If you change the std::array in dingari's example to std::list, then the code also does not compile on clang.

A proper fix would be to add an overload for make_span and span ctor taking two iterators, or to change the two pointers overload to a two iterators overload.

@dingari
Copy link
Author

dingari commented May 13, 2021

@beinhaerter it makes sense that attempting to make span with a std::list iterator pair would fail, since the span should represent a view into a contiguous space in memory. The same should apply to e.g. std::deque, which is essentially just a ring buffer.

I'm guessing the GCC and Clang stdlib implementations are implicitly casting std::array's iterators to pointers and using the pointer-pair overload for make_span.

Nevertheless, having an overload taking iterators and making sure that they're actually pointing to a contiguous area in memory would be really helpful. How that would be achieved is beyond my grasp, but C++20 seems to add std::contiguous_iterator_tag, but I don't know the intricacies of the iterator traits well enough.

For context: in our use-case, we're doing a lot of work with stack-allocated std::array buffers, doing some sort of encoding/copying that returns an end iterator, and passing that range forward as a span.

@JordanMaples I'm up for anything, but I just thought raising a Github issue would suffice, since it seems to be a GSL issue (like @beinhaerter mentioned).

@JordanMaples
Copy link
Contributor

I didn't realize that Clang/GCC gave the option to return as a pointer and assumed it was a different issue. In that case, don't file a Developer Community bug.

In general, we've been recommending against using gsl::make_span in favor of directly using span's constructors directly. However, I see that too is also not working in MSVC because of the same reason. I'll let the other maintainers know about this issue and I'll start looking into making a fix.

If either of you would like to put out a PR to address this, I'll gladly review it.

@beinhaerter
Copy link
Contributor

@beinhaerter it makes sense that attempting to make span with a std::list iterator pair would fail, since the span should represent a view into a contiguous space in memory. The same should apply to e.g. std::deque, which is essentially just a ring buffer.

Absolutely. My comment about the list is totally irrelevant.

@hsutter
Copy link
Collaborator

hsutter commented May 19, 2021

Maintainers call: C++20 includes a contiguous_iterator concept. On a C++20 implementation we could use that concept to enable make_span(iter,iter). We will also look into a C++14-equivalent fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants