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

Document the order of {Vec,VecDeque,String}::retain #60396

Merged
merged 2 commits into from May 12, 2019

Conversation

Projects
None yet
6 participants
@cuviper
Copy link
Member

commented Apr 30, 2019

It's natural for retain to work in order from beginning to end, but
this wasn't actually documented to be the case. If we actually promise
this, then the caller can do useful things like track the index of each
element being tested, as discussed in the forum. This is now
documented for Vec, VecDeque, and String.

HashMap and HashSet also have retain, and the hashbrown
implementation does happen to use a plain iter() order too, but it's
not certain that this should always be the case for these types.

r? @scottmcm

Document the order of {Vec,VecDeque,String}::retain
It's natural for `retain` to work in order from beginning to end, but
this wasn't actually documented to be the case. If we actually promise
this, then the caller can do useful things like track the index of each
element being tested, as [discussed in the forum][1]. This is now
documented for `Vec`, `VecDeque`, and `String`.

[1]: https://users.rust-lang.org/t/vec-retain-by-index/27697

`HashMap` and `HashSet` also have `retain`, and the `hashbrown`
implementation does happen to use a plain `iter()` order too, but it's
not certain that this should always be the case for these types.
@scottmcm

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@rust-lang/libs Was this a part of the expected contract, which just wasn't documented, or does it need an explicit team decision that this is the desired semantic?

Personally it seems reasonable to me; I don't see a good reason to ever do something different for Vec or String. (I could imagine VecDeque doing its two slices differently, one forward and one backward, but that wouldn't improve overall complexity, so probably wouldn't be worth the difference.)

@scottmcm scottmcm added the T-libs label Apr 30, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Let's find out...

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Apr 30, 2019

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

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.

@rfcbot

This comment has been minimized.

Copy link

commented May 1, 2019

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

@scottmcm

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@cuviper The comment changes here look good to me; want to also add some tests for this while we wait for the FCP to complete? From the quick look I did, the tests/examples don't see to exercise the FnMut-ness, nor check the order.

@cuviper

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

OK, I've added an ordered example to each.

@rfcbot

This comment has been minimized.

Copy link

commented May 11, 2019

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.

The RFC will be merged soon.

@scottmcm

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Thanks, @cuviper!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

📌 Commit 0545375 has been approved by scottmcm

@bors

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

⌛️ Testing commit 0545375 with merge 16e356e...

bors added a commit that referenced this pull request May 12, 2019

Auto merge of #60396 - cuviper:ordered-retain, r=scottmcm
Document the order of {Vec,VecDeque,String}::retain

It's natural for `retain` to work in order from beginning to end, but
this wasn't actually documented to be the case. If we actually promise
this, then the caller can do useful things like track the index of each
element being tested, as [discussed in the forum][1]. This is now
documented for `Vec`, `VecDeque`, and `String`.

[1]: https://users.rust-lang.org/t/vec-retain-by-index/27697

`HashMap` and `HashSet` also have `retain`, and the `hashbrown`
implementation does happen to use a plain `iter()` order too, but it's
not certain that this should always be the case for these types.

r? @scottmcm
@bors

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: scottmcm
Pushing 16e356e to master...

@bors bors added the merged-by-bors label May 12, 2019

@bors bors merged commit 0545375 into rust-lang:master May 12, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@cuviper cuviper deleted the cuviper:ordered-retain branch May 17, 2019

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.