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

Add a trait for checked indexing #2

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@tbu-
Copy link

tbu- commented Sep 11, 2015

No description provided.

@tbu-

This comment has been minimized.

Copy link
Author

tbu- commented Sep 11, 2015

I would like to bikeshed the name a bit, checked_index and checked_index_mut are quite long, I'd find shorter names preferable.

@bluss

This comment has been minimized.

Copy link
Owner

bluss commented Sep 11, 2015

The usual name is get and get_mut, like slices do it. This was mentioned on irc today by @Kimundi (not w.r.t arrayvec).

@tbu-

This comment has been minimized.

Copy link
Author

tbu- commented Sep 11, 2015

These names are already taken though. Maybe get_ and get_mut_?

@bluss

This comment has been minimized.

Copy link
Owner

bluss commented Sep 11, 2015

Maybe checked index is good then. If we extend this to &str by the way, the has_index check needs to include character boundary checks (is_acceptable_index in odds).

I don't think has_index method should have any panicking case?

@tbu-

This comment has been minimized.

Copy link
Author

tbu- commented Sep 11, 2015

has_index currently has the panicking case that start > end, but I consider that acceptable as that indicates a programmer error.

I have a small error there, it will currently panic if you use start.. but it is out of range. In that case it panics.

It would need to include boundary checks for &str, yeah. Can we do that in a follow-up PR?

@tbu-

This comment has been minimized.

Copy link
Author

tbu- commented Sep 11, 2015

Fixed the mistake.

Thanks for your fast response times by the way, it's wonderful!

@bluss bluss self-assigned this Sep 12, 2015

@bluss

This comment has been minimized.

Copy link
Owner

bluss commented Sep 17, 2015

Good catch.

I know I said I would easily accept this functionality, just imagined it as a simple function or so. I don't know.

I think there should be no possiblity of panic -- I use checked versions in places where you want to avoid panics, a possible panic in the optimizer's analysis may throw it off and miss some other optimizations.

@llogiq

This comment has been minimized.

Copy link

llogiq commented Sep 21, 2015

Why not slice and slice_mut?

@bluss

This comment has been minimized.

Copy link
Owner

bluss commented Sep 21, 2015

It's going to cover both indexing and slicing, just like the Index trait.

@bluss

This comment has been minimized.

Copy link
Owner

bluss commented Sep 21, 2015

Maybe we can use just something like this?

I'd like to not reuse the Index impls since we want a no-panic codepath anyway.

trait CheckedIndex<Idx> {
    type Output;
    fn checked_index(&self, i: Idx) -> Option<&Self::Output>;
}
@bluss

This comment has been minimized.

Copy link
Owner

bluss commented Sep 21, 2015

Optionally: Even add len() into the trait. That would put us close to a "Random Access (Iterator)" kind of interface.

@tbu-

This comment has been minimized.

Copy link
Author

tbu- commented Sep 21, 2015

The optimizer has no problem with seeing the double bounds check, it will eliminate the panic anyway – I don't think not re-using Index is a good idea, as it just produces code duplication.

@bluss

This comment has been minimized.

Copy link
Owner

bluss commented Sep 21, 2015

That's a good point, it's true most of the time.

@bluss

This comment has been minimized.

Copy link
Owner

bluss commented Sep 21, 2015

I'm inclined to close this, since I don't agree with the approach. The crate space is free — it doesn't have to be in odds in particular.

@tbu-

This comment has been minimized.

Copy link
Author

tbu- commented Sep 21, 2015

That's fine with me!

@tbu- tbu- closed this Sep 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.