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

RFC for replacing slice::tail()/init() with new methods #1058

Merged
merged 4 commits into from Jul 8, 2015

Conversation

Projects
None yet
@lilyball
Copy link
Contributor

lilyball commented Apr 12, 2015

Rendered.

A PR for this has already been submitted as rust-lang/rust#24184.

@lilyball

This comment has been minimized.

Copy link
Contributor Author

lilyball commented Apr 12, 2015

There is some existing discussion in rust-lang/rust#24141 and rust-lang/rust#24184

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Apr 12, 2015

I really like the tail and init names. shift_* suggest these methods mutate the underlying slice somehow, which is not a case.

@netvl

This comment has been minimized.

Copy link

netvl commented Apr 12, 2015

The basic idea is nice, but I personally don't like shift names. They indeed have a mutation connotation. init and tail have pretty widespread meaning. Maybe something like split_tail/split_init would be better?

@Florob

This comment has been minimized.

Copy link

Florob commented Apr 13, 2015

I like the general approach. In terms of bike-shedding I think I'd prefer shift_last() to return Option<(&[T], &T)>, since that seems like a more natural order.
Personally I think the shift_* naming is fine, however I'm probably biased since I'm still used to them from the times when slices actually had a lot of those, which are apparently all gone now.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 13, 2015

The basic idea is great! I prefer split_ as a prefix, and reversing the tuple for the *last* versions, too.

@lilyball

This comment has been minimized.

Copy link
Contributor Author

lilyball commented Apr 13, 2015

How do you feel about split_first() / split_last()? I'd really like to move away from the init / tail naming; the methods are already unstable with a comment indicating they're like to be renamed, and they have unwanted historical baggage (expectation of a head method, and panicking on an empty slice instead of returning an optional value).

With the name shift_last() I think Option<(&T, &[T])> makes more sense, as the idea is "remove the first/last element and return it, along with the remainder", but with the name split_last() I agree that Option<(&[T], &T)> would be more sensible ("split" implies a preservation of existing order).

Add split_first/split_last to unresolved questions
Add the suggested `split_` prefix as a name option, albeit with the
names `split_first()`/`split_last()` instead of the originally-suggested
`split_init()`/`split_tail()`.

Add a question about the return type of `shift_last()`.
@lfairy

This comment has been minimized.

Copy link
Contributor

lfairy commented Apr 14, 2015

I prefer split_first() / split_last() for the same reasons as everyone else (it doesn't suggest mutation).

I'm aware that the word "shift" appears in slice_shift_char(). But that method isn't named shift_char() -- it has an extra slice prefix tacked on. So I don't think it counts as precedent for a shift_* prefix.

In that vein, I think users will be more familiar with split() and split_at(). So if we take consistency as a goal, then we should be consistent with these methods first.

In fact I'm all for renaming slice_shift_char() to split_first() as well, though I understand if you want to restrict the scope of this RFC.

@iopq

This comment has been minimized.

Copy link
Contributor

iopq commented Apr 14, 2015

I feel init() and tail() are inconsistent names because I always look for head() in the API

@blaenk

This comment has been minimized.

Copy link
Contributor

blaenk commented Apr 14, 2015

@iopq: If you're looking for head(), then what you want is first(). init() and tail() do have precedent in other languages (not that that should be a reason for keeping them), such as Haskell.

Personally, I do prefer the existing names due to precedent, but I'm fine with the proposed names!

@iopq

This comment has been minimized.

Copy link
Contributor

iopq commented Apr 14, 2015

if there is something called first() then I would assume the rest is called rest()

if there is something called head() then I would assume the rest is called tail()
I don't care for mixed metaphors

@blaenk

This comment has been minimized.

Copy link
Contributor

blaenk commented Apr 14, 2015

I see what you mean now. I agree.

@rkjnsn

This comment has been minimized.

Copy link
Contributor

rkjnsn commented Apr 14, 2015

I like the idea and the return values. I like the single element being returned first in both cases. I have no preference as to shift vs split.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Apr 16, 2015

assigning to @aturon (i.e. to shepherd).

The expression `slice.shift_last().unwrap.1` is more cumbersome than
`slice.init()`. However, this is primarily due to the need for `.unwrap()`
rather than the need for `.1`, and would affect the more conservative solution
(of making the return type `Option<&[T]>`) as well.

This comment has been minimized.

@Gankro

Gankro Jun 11, 2015

Contributor

This seems like a non-sequitur. The idiomatic translation is:

  • slice.tail() -> &slice[1..]
  • slice.init() -> &slice[..slice.len() - 1]

No? Basically init/tail have really just been superceeded by the vastly more flexible slicing syntax (we don't even need to have _mut variants because of slicing, woo!)

This comment has been minimized.

@iopq

iopq Jun 14, 2015

Contributor

Couldn't you potentially typo and write &slice[..slice.len()]?

This comment has been minimized.

@lilyball

lilyball Jul 1, 2015

Author Contributor

&slice[..slice.len() - 1] only works if you have the slice stored in a variable. If it's the result of some expression, you need to introduce a temporary variable to do that. Which is why I used slice.shift_last().unwrap.1 instead.

This comment has been minimized.

@lilyball

lilyball Jul 1, 2015

Author Contributor

Also to note, the &slice[..slice.len()-1] alternative was given up above on line 45, this drawback is only calling out the awkwardness of the shift_last() approach.


# Drawbacks

The expression `slice.shift_last().unwrap.1` is more cumbersome than

This comment has been minimized.

@Gankro

Gankro Jun 11, 2015

Contributor

unwrap()


```rust
if let Some((_, init_fields)) = variant.fields.shift_last() {
for field in init_fields {

This comment has been minimized.

@Gankro

Gankro Jun 11, 2015

Contributor

vs

let len = variant.fields.len();
if len > 0 {
    for field in &variant.fields[.. len - 1] {

vs

variant.fields.shift_last().map(|(_, init_fields)| {
   for field in init_fields {

(I find if-let to be really noisy, especially when working with options)

@Gankro

This comment has been minimized.

Copy link
Contributor

Gankro commented Jun 11, 2015

Misc thoughts:

  • I agree that split_* is perhaps a better name, as this is entirely what we're doing, although it is a bit unfortunate that the semantics with split_at are quite different (panic).
  • I'd like to see an alternative of "just deprecate these" as their current design is almost entirely redundant with slicing (modulo not having to retrieve the len for init)
  • I agree that re-ordering split_last makes sense

I am not super sold on having methods for this overall, but I acknowledge that there's something intrinsically special about the first and last elements in lists.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 1, 2015

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

@jongiddy

This comment has been minimized.

Copy link

jongiddy commented Jul 6, 2015

One reason to keep the single element as the first return value for split_last is to easily reuse code with either split_first or split_last. I could imagine selecting FIFO or LIFO order by changing from shift_first to shift_last for example.

@jarcane

This comment has been minimized.

Copy link

jarcane commented Jul 6, 2015

first/rest, car/cdr, head/tail are pretty common sequence idioms in other languages. They're a useful pattern, albeit one that is admittedly more useful where recursion is available and efficient.

Furthermore, I can't say as these alternatives seem particularly palatable:

    slice.tail() becomes &slice[1..]
    slice.init() becomes &slice[..slice.len()-1] or slice.shift_last().unwrap().1

Given the choice, I'd choose the former, though a more consistent set of either head/tail, or first/rest, would be a good idea.

@huonw

This comment has been minimized.

Copy link
Member

huonw commented Jul 6, 2015

I'd be interested if we could get a bit of a survey of how these functions are used more broadly than just in the main distribution. That said, a quick glance at the github search results... turned up essentially nothing. (I saw one use of tail, but it was in old code and was paired with a head() call, so would be "neater" with the functionality in this RFC anyway.)

These are certainly useful for "triangular" iteration:

fn foo(mut v: &mut [T]) {
    while let Some((head, rest)) = v.split_first_mut() {
        for x in rest { pairwise_mut(head, x) }
        v = rest;
    }
}

(I'm in favour of split instead of shift, because the latter feels like it should mutate, i.e. fn shift_first<'a>(x: &mut &'a [T]) -> &'a T that updates *x to be shorter.)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 8, 2015

The consensus of the libs team was to merge this RFC with the shift namings renamed to split. Could you update this @kballard and then ping me to merge? Thanks!

@lilyball

This comment has been minimized.

Copy link
Contributor Author

lilyball commented Jul 8, 2015

@alexcrichton Updated.

@alexcrichton alexcrichton merged commit a0d4344 into rust-lang:master Jul 8, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 8, 2015

@chriskrycho chriskrycho referenced this pull request Feb 8, 2017

Closed

Document all features in the reference #38643

0 of 17 tasks complete

@chriskrycho chriskrycho referenced this pull request Mar 11, 2017

Closed

Document all features #9

18 of 48 tasks complete

@Centril Centril added the A-slice label Nov 23, 2018

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.