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 panic-safe indexing methods round 2 #1679

Merged
merged 6 commits into from Aug 16, 2016

Conversation

Projects
None yet
8 participants
@sfackler
Copy link
Member

commented Jul 15, 2016

This is #1325 with the changes discussed at the bottom of that comment chain applied.

@sfackler sfackler added the T-libs label Jul 15, 2016

@sfackler sfackler self-assigned this Jul 15, 2016

@aturon

This comment has been minimized.

Copy link
Member

commented Jul 15, 2016

I'm in favor of overloading get and friends in some fashion (so basically, the goals here). But as @sfackler points out, the precise trait used here is unfortunate.

Basically what's going on here is a form of "double dispatch", where the thing we want to call depends on two types (the slice, and the index/range/...). Often when we do double-dispatch, we do it with a single-method trait. So one alternative would be having on trait per method here, which would eliminate one of the main drawbacks of the proposed trait, and would allow us to stabilize. OTOH, that's likely even harder to understand on the docs side.

(Update: @sfackler added this as an explicit alternative.)

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

I think it may also be worth mentioning a drawback of how the documentation here will get a little worse (as @aturon mentioned). Additionally, these are pretty core types and I'd personally be much more confident if we could try this out at least a little, @sfackler could you prepare a branch with this change that make check passes on and I can try running it on crater?

@sfackler

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2016

Ah right, added a drawback about docs.

@brson

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2016

Seems smart.

I think this could be more explicit about the final syntax enabled by this change and how it achieves that. There's some indirection here that isn't obvious. What I think this RFC does is make [T].get() work with range arguments like [T].get([a..b]).

Will this cause any type inference to get worse?

@sfackler

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2016

I'm working on a branch to crater here: https://github.com/sfackler/rust/tree/slice-get-slice

@sfackler

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2016

@brson I'll expand the overview tonight.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 19, 2016

@sfackler awesome, thanks! Feel free to ping me whenever that's ready to go and I'll start a run.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2016

I have thought from time to time that I would really like to have something like the SliceIndex trait. I recognize its limitations (tied, as you say, directly to the operations it supports) -- but I do think it'd be nice to stabilize eventually.

For example, I often make newtyped indices, and I think I would like to have them work just like other indices. It is possible of course to wrap a Vec<T> with something like IndexVec<I,T>, where I is the index type, and that is more type-safe, but it's also annoying, because IndexVec must mirror the full vector API, and I've not found that it adds a lot over the newtyped index.

@sfackler

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2016

@alexcrichton ok, I think the branch is ready to go. I ran into run-pass-valgrind failures trying to test it myself, but I don't think that's related to my changes.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 19, 2016

Crater build has been scheduled

@sfackler

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2016

@brson I pushed some additional text - does it seem better now?

@nikomatsakis Yep, it does seem like we'd want to stabilize the ability to add indices eventually. I do agree with @aturon that if we did stabilize we'd want to split out a trait per method. I'd be fine doing that now, but it is a slightly bigger surface area.

@brson

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

@sfackler Yes, awesome.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

🔔 This RFC is now entering its week-long final comment period 🔔

I'll be sure to do a crater run before we move to merge this to ensure there's no accidental breakage

@ubsan

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2016

I'd be personally more in favor of trait Get<T> { ... }, trait GetMut<T>: Get<T> { ... }, trait GetUnchecked<T>: Get<T> { ... } trait GetUncheckedMut<T>: GetUnchecked<T> + GetMut<T> { ... }, and implementing them for types that have getters, similar to the index traits. I'd also be more in favor of having the 4 separate traits. However, I'm still in for this RFC.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 16, 2016

Discussed during libs triage yesterday the conclusion was to merge as-is. It'll be good to get the stable interface of these methods out there and before (if ever) stabilizing the backing traits we can always revisit the one or multi-trait question again.

Thanks again for the RFC @sfackler!

@alexcrichton alexcrichton merged commit 293adaf into rust-lang:master Aug 16, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 16, 2016

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.