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

Convert all uses of vec::slice to vec::view #3869

Closed
bstrie opened this issue Oct 26, 2012 · 16 comments
Closed

Convert all uses of vec::slice to vec::view #3869

bstrie opened this issue Oct 26, 2012 · 16 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Milestone

Comments

@bstrie
Copy link
Contributor

bstrie commented Oct 26, 2012

16:38 <@nmatsakis> of course we plan to remove vec::slice()
16:38 <@nmatsakis> but no one has done it yet :)
16:39 <@nmatsakis> vec::view(x, 0, 5).to_vec()
16:39 <@nmatsakis> is basically equivalent to vec::slice(x, 0, 5) today
16:45 < bstrie> nmatsakis: is that the extent of the transformation required to go from slice to view?
16:46 <@nmatsakis> bstrie: I believe so
16:46 < bstrie> is vec::view also defined as a method on the same types that vec::slice is defined on?
16:47 <@nmatsakis> yes.
@graydon
Copy link
Contributor

graydon commented Oct 27, 2012

Huh. Maybe at an interface level, but I would quite like the resulting function to be called slice. I mean, the type that it returns is the slice type, &[], right? Seems like the correct term.

@nikomatsakis
Copy link
Contributor

Yes, I think what we actuallly want to do is (1) replace vec::slice() with vec::view() and (2) rewrite existing uses of vec::slice() to use the pattern foo.slice().to_vec() if they actually need a new vector, and just foo.slice() if a slice will suffice.

@nickdesaulniers
Copy link

I'll tackle this if niko's suggestion is the consensus. Can someone assign this to me if it's good?

@bstrie
Copy link
Contributor Author

bstrie commented Jan 18, 2013

Not sure what the status of this currently is, but in fact just today there was someone in the IRC channel who was halfway to reimplementing .view because .slice was doing the wrong thing. I'd say go ahead with it, seems like it wouldn't be entirely difficult to make this sort of mechanical change.

@nickdesaulniers
Copy link

So it sounds like they've already started on this bug?

@bstrie
Copy link
Contributor Author

bstrie commented Jan 18, 2013

@nickdesaulniers no, I meant that they were accidentally reimplementing slices in their own code because they had thought that .slice was Rust's notion of a slice type, when actually .view is the canonical slice type. What I mean is that this issue should be settled ASAP to minimize the amount of confusion this continues to cause.

@nikomatsakis
Copy link
Contributor

Nick, I can't seem to assign you, however I think there is no reason not
to start work on it.

@nickdesaulniers
Copy link

Are vec::view and and vec::slice being swapped, or are we discarding the current implementation of vec::slice and renaming vec::view to vec::slice?

@nikomatsakis
Copy link
Contributor

I favor just having one method.

@nickdesaulniers
Copy link

Just to triple check; prior to any changes, the function signatures look like:

pub pure fn slice<T: Copy>(v: &[const T], start: uint, end: uint) -> ~[T];
pub pure fn view<T>(v: &r/[T], start: uint, end: uint) -> &r/[T];

and after:

pub pure fn slice<T>(v: &r/[T], start: uint, end: uint) -> &r/[T]

I've been working through necessary changes to other places that call slice or view. One issue I'm running into is a few functions such as tail:
https://github.com/mozilla/rust/blob/incoming/src/libcore/vec.rs#L227
which passes a first argument to slice of type: &[const T] but slice's new function signature from my patch requires the first argument to be of type &[T] so I get an error: mismatched types: expected &/[]but found&/[const 'a] (values differ in mutability). I don't think changing slice's new signature to take a borrowed reference to a vector of const T's is a good idea, since elsewhere in vector, there are warnings against passing that type to as_imm_buf:
https://github.com/mozilla/rust/blob/incoming/src/libcore/vec.rs#L1475

I don't think it would be a good idea for me to modify vec::tail, vec::tailn, vec::init, but I'm not in a position to make that call. Thanks in advance for the help!

@nickdesaulniers
Copy link

note: rename mut_view to mut_slice, and const_view to const_slice

@nickdesaulniers
Copy link

Tests are green for view -> slice rename. Rebasing, then renaming const_view and mut_view

@nickdesaulniers
Copy link

All done, tests are green. Going to rebase this once more and double check the tests, then pull request inbound.

@nickdesaulniers
Copy link

Tests green, pull request inbound

bors added a commit that referenced this issue Feb 15, 2013
Issue #3869
review? @nikomatsakis 

Convert all uses of vec::slice to vec::view Issue #3869
Rename const_view to const_slice
Renamed mut_view to mut_slice
Fix windows build error.  `buf` is borrowed by the call to
`as_mut_buf()` and so we must invoke `slice()` outside of that
call.
@nickdesaulniers
Copy link

This issue may now be closed. @brson

@brson
Copy link
Contributor

brson commented Feb 16, 2013

\o/

@brson brson closed this as completed Feb 16, 2013
bors pushed a commit to rust-lang-ci/rust that referenced this issue May 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

5 participants