Uh oh!
There was an error while loading. Please reload this page.
add fn make_contiguous to VecDeque - #69425
Conversation
rust-highfive
commented
Feb 24, 2020
r? @KodrAus (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
commented
Feb 24, 2020
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
62b7f1c to
63ff9a9Comparerust-highfive
commented
Feb 24, 2020
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
KodrAus
left a comment
There was a problem hiding this comment.
Thanks for working on this @lcnr!
The extra ASCII diagrams of our start and end states are helpful! I think it would also be good to add some prose to the comments in each branch of the algorithm too, like the old code had when the centre has room for either the head or tail.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
lcnr
commented
Mar 11, 2020
Added a short comment explaining whats going on, is there anything else I should mention? |
rust-highfive
commented
Mar 11, 2020
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
lcnr
commented
Mar 11, 2020
seems like the file length ended up being less than 3000 after some unrelated changes |
Dylan-DPC-zz
commented
Mar 21, 2020
r? @Amanieu |
Uh oh!
There was an error while loading. Please reload this page.
Mark-Simulacrum
commented
Mar 21, 2020
I think it's true that our only use case right now is essentially And I think the discoverability point that @KodrAusmentions is an important one, I would suggest at least having all of the (current) sort functions on slices called out by name in the documentation for make_contiguous so that if someone is on the VecDeque docs page and searches for it they land on that function. One thought is that we could even implement DerefMut for VecDeque to slice but that seems too likely to cause accidental problems; ideally I think we would be able to show in rustdoc somehow that "all of these methods are available but you need to call this one to get them". I think we should also mention make_contiguous in the top-level comment describing VecDeque... and maybe also call out the into/from Vec conversions being zero-cost after the initial "make_contiguous" call. |
@Mark-Simulacrum pedantic: conversions to Vec are not actually zero cost after ....ABCDEF....which still needs to be shifted to the start of the buffer, which has a runtime of Will update the PR tomorrow. I don't feel 100 % comfortable recommending this method in the struct docs while it is unstable though. |
Mark-Simulacrum
commented
Mar 21, 2020
Ah, yes, that's a good point -- let's avoid the top-level mention (but leave a note in the tracking issue) but do call out sort/sort_by/etc in the function-level documentation. Interesting, I guess my brief look missed that make_contiguous is slightly more efficient than the Vec conversion. Nice! |
lcnr
commented
Mar 22, 2020
Sorting is now explicitly mentioned in the documentation, and btw, can we use something like |
Amanieu
commented
Mar 22, 2020
Can you update the documentation for |
lcnr
commented
Mar 22, 2020
Done, not completely satisfied with the wording though. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-Authored-By: Amanieu d'Antras <amanieu@gmail.com>
Amanieu
commented
Mar 31, 2020
@bors r+ |
bors
commented
Mar 31, 2020
📌 Commit e1afd26 has been approved by |
Rollup of 7 pull requests Successful merges: - rust-lang#69425 (add fn make_contiguous to VecDeque) - rust-lang#69458 (improve folder name for persistent doc tests) - rust-lang#70268 (Document ThreadSanitizer in unstable-book) - rust-lang#70600 (Ensure there are versions of test code for aarch64 windows) - rust-lang#70606 (Clean up E0466 explanation) - rust-lang#70614 (remove unnecessary relocation check in const_prop) - rust-lang#70623 (Fix broken link in README) Failed merges: r? @ghost
Adds the following method to VecDeque:
Taken from #69400, after a suggestion by @CryZe#69400 (comment)
I am in favor of merging this instead of #69400.