Document the order of {Vec,VecDeque,String}::retain #60396
Conversation
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.
rust-highfive
added
the
S-waiting-on-review
|
@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 |
scottmcm
added
the
T-libs-api
|
Let's find out... @rfcbot fcp merge |
|
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
added
proposed-final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
@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 |
|
OK, I've added an ordered example to each. |
|
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. |
rfcbot
added
finished-final-comment-period
|
📌 Commit 0545375 has been approved by |
bors
added
S-waiting-on-bors
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
|
☀️ Test successful - checks-travis, status-appveyor |
|
I believe unit tests to ensure the retain order is missing: rust/src/liballoc/tests/vec.rs Line 226 in 70456a6 |
|
I think the doc tests suffice, but you could add more standalone tests if you like. |
The examples added in rust-lang#60396 used a "clever" post-increment hack, unrelated to the actual point of the examples. That hack was found [confusing] in the users forum, and rust-lang#81811 already changed the `Vec` example to use a more direct iterator. This commit changes `String` and `VecDeque` in the same way for consistency. [confusing]: https://users.rust-lang.org/t/help-understand-strange-expression/62858
…lett Update the examples in `String` and `VecDeque::retain` The examples added in rust-lang#60396 used a "clever" post-increment hack, unrelated to the actual point of the examples. That hack was found [confusing] in the users forum, and rust-lang#81811 already changed the `Vec` example to use a more direct iterator. This commit changes `String` and `VecDeque` in the same way for consistency. [confusing]: https://users.rust-lang.org/t/help-understand-strange-expression/62858
It's natural for
retainto work in order from beginning to end, butthis 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, andString.HashMapandHashSetalso haveretain, and thehashbrownimplementation does happen to use a plain
iter()order too, but it'snot certain that this should always be the case for these types.
r? @scottmcm