-
Notifications
You must be signed in to change notification settings - Fork 738
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
Add span_iterator overloads for make_span() and subspan() #461
Comments
span_iterator
overloads for make_span()
and subspan()
Maybe a separate library would be more appropriate? (Or at least a separate section in this library) There must be quite a few basic functions that people would use a lot. For example splitting an array into Your example could return a template<typename T>
std::pair<gsl::span<T>, gsl::span<T>> bisect(gsl::span<T> s, T const& v)
{
auto found = std::find(s.data(), s.data() + s.size(), v);
return {{s.data(), found}, {found, s.data() + s.size()}};
} |
If we define template <class T>
class span
{
public:
using iterator = T*; Then code becomes simpler and more practical. Therefore what's the point of |
You could simply use a different span implementation. No-one forces you to use bound-checked iterators. For example: https://github.com/ericniebler/range-v3/blob/master/include/range/v3/span.hpp |
Ok so the goal of My goal is, if possible, help to improve the GSL / span proposal and make it better. And I think there's a hole in the current spec that allows 2 different implementations to be easily incompatible. After a quick read of the pdf, it's not clear to me what the following code should do: span<int> f(span<int> s)
{
return make_span(std::next(s.begin()), s.end());
} Based on how I think this point should be clarified, and I wished it would require the code to compile because it makes the interface way more useful (IMO) and won't require much work in the current implementation (1 extra overload for |
Doesn't really make a difference, but there have been a few revisins latest paper I could find on this is http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0122r4.pdf. In my personal opinion, working with gsl::span is anyway much less pleasant than one would expect. So if range checking is not deemed important (although this is actually a core idea behind the span proposal) then the ranges-v3 version is a good alternative - even if it comes with a lot of baggage. |
Ok. So I'll create a pull request with my proposed changes and we will see. Until now in my code I have been using a custom iterator pair class (make_range), a similar idea than what proposed @galik. I saw the limits of that, and I decided to see how That being said, I identified another caveat with the current implementation of class X {
public:
span<int> get() const { return v; }
private:
std::vector<int> v{1, 2, 3, 4, 5};
};
int main() {
X x{};
for (auto it = x.get().begin(), end = x.get().end(); it != end; it++) {
it = std::find(it, end, 3);
}
} It crashes because I think this implementation detail could be challenged and replaced with another one that it not tied to a |
This is a design decision, which was discussed in another issue already. |
Hello @aurelienrb and thanks for taking the time to raise an issue. Sorry for the delay in getting back to you. I was off at a C++ standards meeting and then took some vacation. Now, there's a lot going on in this thread, so let me try to summarize my responses to some of the statements being thrown out:
With that said, I've been doing some work recently to improve the implementation of the existing constructors. I think I could extend that work to allow construction from a pair of iterators where the iterators either originated from a In passing, I'm not sure it's reasonable to describe the There are areas with |
A bit off topic, but I'm really glad to hear that |
Hello Neil, First of all, thanks for taking time to comment on my usage of "half-baked", which was completely inapropiate and makes me feel ahamed of :/ English is not my native tongue, and I wasn't aware of how rude and agressive this phrase actually is. My intent was to express the idea that only half of the functions related to iterators are available (accessing data) and thus the API is missing the other half (construction from iterators) to "taste really good". I am even more sorry for such rude words since I know who you are and how much work it requires to improve the language. So please "excuse my French" :) Now regarding the acceptance of iterators in the ctor (or PS: no worries for the "delay": 2 days is fine... I just hope you had time to see a bit more than the Bongo Ben's of Kona :) |
Thanks for clarifying @aurelienrb, no need for embarrassment. Plus, your English is much better that my French could ever hope to be :-) Yes, I do plan to support Yes, I did manage to make it past Bongo Ben's! In fact, I even got a chance to snorkel with some manta rays, which was a mind-blowing experience. Your comment really made me laugh though, as I have spent quite a bit of time at Bongo Ben's on a previous visit ;-) For everyone looking for the latest span proposal text: I have added it to the CppCoreGuidelines repository here: https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/P0122R4.pdf I will keep that updated with future revisions, and it will eventually be referenced in nicer GSL documentation over at that repo. |
Ok...so since this issue was last discussed things have moved on quite a bit. I'm going to close the issue out as - unless the standard library adds a way to identify contiguous iterators - using that constructor is the simplest and best option available for now. |
Hello,
I often need to search for a value in a
span
and split where it was found. And I can't use the standard algorithms such asstd::find()
because they return aspan_iterator
... that I can do nothing with!Therefore I propose to introduce overloads for
make_span()
andsubspan()
that allow to write code like that:What's your opinion?
Aurelien
The text was updated successfully, but these errors were encountered: