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

make src into a range and get rid of count #1

Merged
merged 3 commits into from
Sep 17, 2018
Merged

Conversation

oconnor663
Copy link
Owner

This is an example implementation of a change @SimonSapin suggested in rust-lang/rust#53652 (comment).

@SimonSapin
Copy link

(Note that this is ~half of my suggestion, I also proposed changing dest from usize to RangeFrom<usize> to make it clear that it is the start position of some range. (However it is not dest.start()..buffer.len() which is the usual meaning of RangeFrom, so maybe that’s not such a good idea…)

@oconnor663
Copy link
Owner Author

Here's an example instantiation to look at the difference: https://godbolt.org/z/PsxH7K

It seems like a few instructions get added to the caller in this case, probably too small of a difference to be measurable?

src/lib.rs Outdated
Bound::Unbounded => 0,
};
let src_end = match src.end_bound() {
Bound::Included(&n) => n.saturating_add(1),
Copy link
Owner Author

@oconnor663 oconnor663 Sep 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is technically correct. If a slice actually ends at the largest possible address (I think that's one less than the maximum representable, to allow C to compute the p + len pointer?), then an inclusive range that passes the end of the slice by one would be converted from the panic it should be to a valid range one element shorter than the caller asked for.

@oconnor663
Copy link
Owner Author

Playing with this a little more, I think it's obviously better to read and write than my original API. I'm going to land it here and update the Rust PR.

@oconnor663 oconnor663 merged commit e54785c into master Sep 17, 2018
@oconnor663 oconnor663 deleted the rangebounds branch September 17, 2018 19:40
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.

2 participants