Stabilize VecDeque::retain_back from truncate_front

[cammeresi]

View all comments

Tracking issue: #140667
Closes: #140667

@rustbot label -T-libs +T-libs-api +needs-fcp +S-waiting-on-fcp

r? t-libs-api

[cammeresi]

@BurntSushi I know process is required before this patch can be approved but is a pull request the correct way to do it or is there another way?

[cammeresi]

@rustbot reroll

[Amanieu]

@rfcbot merge libs-api

[rust-rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @the8472

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[dtolnay]

Some alternatives to consider:

1. Add no method, change truncate's doc to indicate that while you can only use truncate to retain front elements, you can retain the back elements by using vecdeque.drain(..idx). Prior art: for slices we recently chose to expose front truncation using slice.split_off(..idx) which takes a OneSidedRange.

2. Instead of putting this in the truncate family, put it in the retain family with a name like vecdeque.retain_last(N). This eliminates ambiguity about whether the argument for truncate is supposed to be an index vs a number of elements to retain, which are 2 correct mental models that different people may have for truncate, one of which does not translate correctly to truncate_front.

I would be okay with any of the 3 designs.
@rfcbot reviewed

[joshtriplett]

After further consideration, I think I'd be fine shipping this as-is. I think the "retain" family isn't as good a fit; that family generally involves a predicate.

I do find the idea of steering people towards something range-based rather than truncate somewhat appealing; there's something nice about having people say "this is the range I want". But that isn't the ideal API for Vec, for instance, where anything other than ..end or ..=last would be inefficient and require a memory move; truncate makes more sense there. And for people familiar with truncate, it's nice for VecDeque to provide both truncate and truncate_front.

On that basis, I think it makes sense to stabilize this. And in addition, I'd personally love to see an API for "keep only this specified range", rather than adding any kind of truncate_both.

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[cammeresi]

@joshtriplett The idea of retaining only an inner range is an interesting idea. Do you have any example uses in mind?

The best I could imagine is collecting metrics, sorting by a field, then discarding some of the top and bottom as unrepresentative. But the sort requires going through make_contiguous currently, which makes the example unlikely to exercise all possible code paths and so a little contrived, though one could imagine a future wherein VecDeque could sort itself directly without having to coördinate through make_contiguous and slice.

[urben1680]

In a bevy crate I have rather complex deques that contain data for undo-redo actions. The collection also contains an index that defines which portion of it is the "future" segment. For example if I do not push but do undo, I get a reference of the value at index and the index is reduced. So the range after that index is considered the "future", the other elements the "past". When updating the deque with push, I also provide a maximum past length. And there I basically retain only an inner range: the future segment and any more elements of the past end that exceeds the maximum length have to go.

Though my case is even more complex because I also provide draining iterators of the past and future at this method, which I do by draining all of the deque, collect into a boxed slice if the iterator gets to the segment I want to keep, and on Drop reinsert them + the newly pushed value into the deque.

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[thaliaarchi]

It feels like the name change was snuck in hastily post-FCP. I think the analogy with truncate, which takes a length, is much stronger than retain, which takes a predicate. What's the avenue to formally discuss this?

[clarfonthey]

Well, since this got added shortly after the beta branch, we seem to have quite a while to discuss this, and I believe that was part of the reason for going ahead so quickly. We have until mid-August before this would get moved to the current beta, and by October before that beta would become version 1.99.0.

Probably the best way would be to start a thread in #t-libs-api/api-changes on Zulip, since that way folks can talk more about it async. But if you aren't on/don't want to use Zulip, you could share some additional thoughts here and we could nominate it for next week's meeting on Tuesday, which you're also free to show up to if you want. (All meetings are open to the public.)