Cppcoreguidelines: GSL Should span iterators from spans to the same data be interoperable?

[suncho]

I frequently use a pattern in my code where I create two identical temporary span objects and pass a begin iterator from one and an end iterator from the other to a standard algorithm (or other functions that work with iterator-defined ranges). The Microsoft GSL implementation doesn't allow this because it does not allow comparison between iterators originating from different span objects. I submitted a pull request to fix this because I thought it was a bug:

https://github.com/Microsoft/GSL/pull/631

What is the intended behavior? @neilmacintosh is worried that allowing this will encourage bad practices.

[hsutter]

Closing for now. If you want us to reopen this, could you please give an example of where you want to pass span1.begin() and span2.end()? That sounds suspicious and error-prone.

[MikeGitb]

@hsutter:

How about

 class my_container {
      span<int> data();
 };

  int main() {
       my_container c{....};
       std::sort(c.data().begin(), c.data().end());
  }

Another problem that I hit back when I wanted to implement range based algorithm wrappers for gsl::span is that you can't usefully pass a span and an iterator by value e.g.

  span<T>::iterator rotate(span<T> range, span<T>::iterator it);

won't work. You have to use

 span<T>::iterator rotate(const span<T>& range, span<T>::iterator it);

Despite gsl::span being supposed to be passed by value (And even then, the user easily falls into the trap mentioned above).

[suncho]

@hsutter

class interesting {
  vector<int> nums;
  // ...
public;
  span<const int> get_nums() const;
  // ...
};

void some_func(const interesting& src)
{
  // Will fail under current GSL
  //auto numbers = vector<int>(begin(src.get_nums()), end(src.get_nums()));

  // Have to do this instead
  auto numspan = src.get_nums();
  auto numbers = vector<int>(begin(num_span), end(num_span));
}

It also comes up in serialization. I have a template function called rep_view that returns a span<const byte> to the representation of an object. copy_bigend is a reverse_copy if our native byte order is little endian:

// This will fail under the current GSL
void serialize_ints(vector<byte>& buf, span<int> ints)
{
  for (int i : ints)
    copy_bigend(begin(rep_view(i)), end(rep_view(i)), back_inserter(buf));
}

// Have to do this instead
void serialize_ints(vector<byte>& buf, span<const int> ints)
{
  for (int i : ints) {
    auto bytes = rep_view(i);
    copy_bigend(begin(bytes), end(bytes), back_inserter(buf));
  }
}

[jwakely]

The same question applies to std::string_view of course.

And if we say it's OK, is it also OK to mix begin and end from non-equal spans/views as long as they're sub-spans of the same thing?

char a[] = "foo"; string_view s = a; string_view t = s; s.remove_suffix(1); t.remove_prefix(1); std::sort(s.begin(), t.end());

This sorts all three characters in a, even though neither s nor t refers to all three characters.

These questions are definitely worth answering.

[neilmacintosh]

@jwakely summarized some of my concerns perfectly. How do you distinguish between mixing iterators from "different" and "the same" views?

[hsutter]

@suncho Thank you for the examples. In both of the examples you gave, I think that creating the named local variable is the right thing to do for now (the best solution is a range constructor and a range-based algorithm): In my opinion it makes the code clearer, avoids recomputing a common subexpression (which is possibly marginally more efficient and something we recommend avoiding anyway, such as similar examples in P.7), and avoids the problem at top.

@MikeGitb Thank you for the examples. In the first, the same reasoning applies: I would say use a local span variable, and the real solution is to use a range-based algorithm. In the second, I agree that passing a span& could be appropriate, but even simpler would be to pass a span by value and an index (instead of an iterator). Did you try that?

@jwakely Thank you for the example. I agree it is a counterexample that we should not support.

So based on these examples I continue to think the status quo is correct, and we should not allow mixing different spans (or string_views) with the assumption they refer to the same thing.

[hsutter]

Reopening as the requested examples were provided (thanks again), for the editors to look at this another time...

In sum, my view is that except for @MikeGitb's second example of (span,iterator)[*], all of these seem to be following an antipattern of taking two spans, using each one as an iterator (i.e., throwing half of its information away) and using the result as a naked iterator pair. That appears to defeat the purpose of span, because that is the undesirable problem style that span is intended to replace. And in all the examples, the real thing we want is ranges (which also eliminates the local variable you want to eliminate).

[*] I think that one would be better as (span,index). However, note that back when we designed span we did consider this use case and wondered whether there was a place for a triplet (span,position) under another name like cursor. We considered a number of use cases from actual code, and I recall it coming up over the course of several weeks of design discussion, but in the end we decided we didn't have sufficient motivation in hand at the time to know we needed it and to know we could it design it accurately as a general facility

[suncho]

@jwakely That's a really good point. I don't know how, in the general case, you'd test whether two iterators were pointing into subspans of the same thing. It turns out that for string_view, Microsoft's iterator checking tests whether the originating views of the two iterators share the same data pointer and the same size. If they do, then it lets you pass.

That's about what I would expect it to do. You could even drop the matching size requirement and it would still only let valid iterator ranges through.

The gsl::span iterators, on the other hand, will save a pointer to the originating gsl::span object. So if you create a copy of that gsl::span, the copy will produce iterators that are incompatible with the original's iterators. And if you assign a gsl::span to a different range or resize a it (e.g. by making it a subspan of itself), that will invalidate previously-created iterators. On the other hand, string_view iterators are only invalidated when the underlying memory gets freed.

I like the string_view behavior better. It feels cleaner. I don't want to have to worry about invalidating some iterators somewhere every time I reuse a gsl::span object for different data.

@hsutter Thanks for the advice. I'm definitely looking forward to ranges.

[hsutter]

Closed again per editors' discussion.