Skip to content

Compare span_iterators to the same data#631

Closed
suncho wants to merge 10 commits into
microsoft:masterfrom
suncho:span-iterator-fix
Closed

Compare span_iterators to the same data#631
suncho wants to merge 10 commits into
microsoft:masterfrom
suncho:span-iterator-fix

Conversation

@suncho

@suncho suncho commented Mar 2, 2018

Copy link
Copy Markdown
Contributor

For example, when span1 and span2 refer to the same data, this allows you to use begin(span1) and end(span2) to form a valid range. It's useful when you're creating identical temporary spans just for the purpose of getting the iterators out:

copy(begin(make_span(weird_container), end(make_span(weird_container), begin(some_vector));

@msftclas

msftclas commented Mar 3, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@suncho

suncho commented Mar 3, 2018

Copy link
Copy Markdown
Contributor Author

span_iterators that refer to the same memory compare neither greater than nor less than each other. If they correspond to spans of different sizes, they do not compare equal to each other either.

A consequence is that the set of span_iterators that compare neither less than nor greater than each other form an equivalence class. This equivalence class is a superset of the equivalence class defined by the equality operator.

This is how how it should be. It ensures that span_iterators are regular. Imagine two span_iterators that are equivalent according to the less-than operator, but that are associated with spans of different lengths. The two iterators will be bounds-checked differently. If the iterators compared equal, then the bounds-checking procedures would not be regular on the span_iterator type.

@neilmacintosh

Copy link
Copy Markdown
Contributor

@suncho This PR doesn't look desirable to me. We do not want to encourage people to compare and mix iterators from different spans, regardless of whether or not they refer to the underlying data. This issue has been discussed before, if I recall correctly. It encourages practices that might just happen to work because of implementation details.

@suncho

suncho commented Mar 4, 2018

Copy link
Copy Markdown
Contributor Author

@neilmacintosh Hmm. What kinds of undesirable practices does it encourage? Can you point me to a previous discussion?

I've found this functionality to be incredibly useful in my own work as I attempt to clean up legacy code and make it safer and easier to read.

See this gist:
https://gist.github.com/suncho/4ccaeb9c69dfef9ffdc0b712f08d85fa

In particular, the wipe function template will fail to work if I replace:

template <class T>
void wipe(T& object)
{
  fill(begin(rep(object)), end(rep(object)), byte(0));
}

with this:

template <class T>
void wipe(T& object)
{
  fill(begin(rep_span(object)), end(rep_span(object)), byte(0));
}

This is because the two calls to rep_span produce different (but equivalent) spans. Under the current Microsoft GSL implementation, the begin iterator is incompatible with the end iterator.

I use this pattern throughout my code when working with byte representations of objects. This way, I can ensure that my byte iterators are range checked. But it only works with spans if we allow iterators from equivalent spans to be compared against one another.

suncho referenced this pull request Mar 6, 2018
* fixed noexept warnings

- Removed conditional compilation for throwing version of GSL
  vs fail_fast because we don't want users of the code to see
  differences in the span interface dependent on error mechanism
  chosen
- Removed noexcept from functions that may fail at runtime
- Fixed CppCoreCheck warnings related to missing and incorrect
  noexcept
- do not warn on unnown attributes for GCC and Clang

* remove suppress that does not compiler for clang and gcc
@suncho suncho force-pushed the span-iterator-fix branch from 7729d30 to 3211f97 Compare April 10, 2018 08:46
@annagrin

Copy link
Copy Markdown

Closing this as the Guidelines discussion desided to go with range-based API.

@annagrin annagrin closed this Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants