-
Notifications
You must be signed in to change notification settings - Fork 85
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
boost::span cannot be constructed with iterators #119
Comments
I don't think adding constructors from iterators is a good idea, unless we can require contiguous iterators. Which means these constructors would only be available in C++20 and onwards. |
It's possible to approximate this by checking for random access and then asserting that |
|
Dereferencing end iterator would be UB. Also, I don't like a runtime assert. The code should simply not compile, otherwise it's too bug-prone IMHO. At most, we could hardcode Let the user be explicit in his intentions - let him explicitly write |
Hence As I said, having the user type the |
Requiring the users to type it makes them aware what they are doing. While accepting iterators and doing it internally could hide a bug. For example, when the span is constructed from an unknown type of range, e.g. in a template function. Note that the assert may still spuriously pass invalid iterators. E.g. from |
Iterators from deque that pass the assert will actually form a valid span.
Only in theory. In practice, all it does is eliminate the opportunity to check iterator category or assert. No awareness will be obtained. |
This would be runtime-dependent. Meaning the code will pass or fail from one run to another, worst kind of bug. When it should just always fail to compile.
I'd like to believe the user is a sane person who knows what and why they are typing. If we really want these constructors in pre-C++20, let's restrict them to being pointers only. |
Yes, but we can't reliably reject it at compile time while allowing the correct cases without C++20. It's obviously a tradeoff that errs towards accepting the correct cases in lieu of rejecting the incorrect cases. |
Actually, let's add a new |
My point is that it is the user who must explicitly say "yes, I know what I'm doing", not the library assume he does. This is standard practice in C++. |
Yes, I did not add these constructors because pre-C++20 it would not be possible to detect contiguous iterators. If users want them in C++20 mode (where we have
|
Thanks a lot for your comments. The best, in my opinion, would be to allow those constructors also in pre-C++20 mode, leaving the user responsible for the code, but I trust your experiences when you say that this is not C++ standard practice. Eventually wrapping the iterators in It would be great to add few lines about this on the documentation, too. |
On second thought, this ( |
That sounds fine. Also fine if the trait is not public ( |
It should be public so that people can specialize it for their iterators (if they insist.) |
I'd rather we didn't specialize Note also, there are |
Not specializing for string and vector makes the whole thing completely useless. We either do it and specialize, or do nothing at all, nothing else makes any sense. |
Specializing for iterators is just not scalable. What about non-std iterators? IMO, specializing only for non- |
No, they can't. It's not possible to specialize the trait for More precisely, it would be technically possible if our trait has an |
They can specialize for their specific |
std::span
constructors 2 and 3 of cppreference.com documentation support generic iterators, whileboost::span
accepts only pointers. A possible workaround is to apply the operators&*
to the iterators.https://godbolt.org/z/sjEozr6M5
The text was updated successfully, but these errors were encountered: