Clarify behavior of `VecDeque::insert`. #38581

frewsxcv · 2016-12-23T20:20:42Z

rust-highfive · 2016-12-23T20:20:56Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

frewsxcv · 2016-12-23T20:21:30Z

I think my changes are an improvement, but I'm not super satisfied with them. Please let me know if anyone has ideas for improvement.

Mark-Simulacrum · 2016-12-23T21:19:45Z

src/libcollections/vec_deque.rs

-    /// end is closer to the insertion point will be moved to make room,
-    /// and all the affected elements will be moved to new positions.
+    /// Inserts an element within the `VecDeque` at index `index`. All elements previously with
+    /// indices greater than or equal to `index` will have their indices incremented by one.


Could we just reuse the Vec comment: "Inserts an element at position index within the vector, shifting all elements after it to the right."? I don't know whether that has different meaning for VecDeque, but seems simpler.

Edit: Link to Vec documentation: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.insert

My hesitation with that comment is that (at least behind the scenes in memory), VecDeque::insert doesn't necessarily shift all the following items to the back like it does with Vec::insert. Though, if we're not talking about what happens in-memory it is true as far as indices/positions go, if that makes any sense. What do you think?

Perhaps "Inserts an element at position index within the vector, incrementing all indices after index by one."

That avoids the problem with somewhat implying the underlying memory operations.

Thanks for the suggestion. I incorporated it in the latest force push.

Though on second thought, your suggestion doesn't really address #37046.

shepmaster · 2016-12-23T21:22:35Z

Looks like your PR / commit message title has a typo: VecDequeue instead of VecDeque.

frewsxcv · 2016-12-24T19:11:16Z

Looks like your PR / commit message title has a typo: VecDequeue instead of VecDeque.

@shepmaster Thanks for pointing that out. It's been fixed now.

Mark-Simulacrum

Looks good, left two minor nits.

Mark-Simulacrum · 2016-12-24T23:41:18Z

src/libcollections/vec_deque.rs

-    /// Inserts an element at `index` within the `VecDeque`. Whichever
-    /// end is closer to the insertion point will be moved to make room,
-    /// and all the affected elements will be moved to new positions.
+    /// Inserts an element at index `index` within the `VecDeque`, shifting all elements with


Maybe just "at index"? "index index" feels a little odd.

Fixed in the latest force push.

Mark-Simulacrum · 2016-12-24T23:42:39Z

src/libcollections/vec_deque.rs

+    /// vec_deque.insert(1, 'd');
+    ///
+    /// let vec = vec_deque.iter().collect::<Vec<_>>();
+    /// assert_eq!(vec, [&'a', &'d', &'b', &'c']);


I'm not sure this will compile, but perhaps we can omit the & in front of each char in the array? Perhaps removing those and making it a slice will work.

That didn't work for me:

error[E0277]: the trait bound `&char: std::cmp::PartialEq<char>` is not satisfied --> <anon>:12:4 | 12 | assert_eq!(vec, &['a', 'd', 'b', 'c']); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::cmp::PartialEq<char>` is not implemented for `&char` | = help: the following implementations were found: = help: <char as std::cmp::PartialEq> = note: required because of the requirements on the impl of `std::cmp::PartialEq<&[char; 4]>` for `std::vec::Vec<&char>` = note: this error originates in a macro outside of the current crate error: aborting due to previous error

Ah, I see the problem. Can you try changing vec_deque.iter().collect::<Vec<_>>(); to vec_deque.into_iter().collect::<Vec<_>>();? That should fix the compile error.

Yep! Fixed in the latest force push.

Fixes rust-lang#37046.

GuillaumeGomez · 2017-01-04T16:01:09Z

Thanks!

@bors: r+ rollup

bors · 2017-01-04T16:01:10Z

📌 Commit 0ab7812 has been approved by GuillaumeGomez

…aumeGomez Clarify behavior of `VecDeque::insert`. Fixes rust-lang#37046.

bors · 2017-01-09T16:02:13Z

⌛ Testing commit 0ab7812 with merge 96d3123...

bors · 2017-01-09T16:06:07Z

💔 Test failed - status-travis

alexcrichton · 2017-01-09T16:52:04Z

@bors: retry * network error

…

On Mon, Jan 9, 2017 at 8:06 AM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/190296475> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#38581 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95PkY7pzNqSujpCBOZU50x7r00Jmbks5rQlrxgaJpZM4LVC9j> .

bors · 2017-01-09T17:31:17Z

⌛ Testing commit 0ab7812 with merge 5c1472a...

Clarify behavior of `VecDeque::insert`. Fixes #37046.

bors · 2017-01-09T19:38:16Z

☀️ Test successful - status-appveyor, status-travis
Approved by: GuillaumeGomez
Pushing 5c1472a to master...

rust-highfive assigned brson Dec 23, 2016

frewsxcv mentioned this pull request Dec 23, 2016

Documentation for VecDeque.insert is unclear #37046

Closed

frewsxcv force-pushed the vecdequeue-insert branch from 1042e8a to 1f6b0b5 Compare December 23, 2016 20:27

Mark-Simulacrum reviewed Dec 23, 2016

View reviewed changes

frewsxcv force-pushed the vecdequeue-insert branch from 1f6b0b5 to c11705e Compare December 24, 2016 19:10

frewsxcv changed the title ~~Clarify behavior of VecDequeue::insert.~~ Clarify behavior of VecDeque::insert. Dec 24, 2016

frewsxcv force-pushed the vecdequeue-insert branch 2 times, most recently from deec978 to 971a019 Compare December 24, 2016 20:15

Mark-Simulacrum reviewed Dec 24, 2016

View reviewed changes

frewsxcv force-pushed the vecdequeue-insert branch from 971a019 to 8f68e23 Compare December 25, 2016 18:23

Clarify behavior of VecDeque::insert.

0ab7812

Fixes rust-lang#37046.

frewsxcv force-pushed the vecdequeue-insert branch from 8f68e23 to 0ab7812 Compare December 26, 2016 08:25

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 4, 2017

Rollup merge of rust-lang#38581 - frewsxcv:vecdequeue-insert, r=Guill…

69888bd

…aumeGomez Clarify behavior of `VecDeque::insert`. Fixes rust-lang#37046.

GuillaumeGomez mentioned this pull request Jan 4, 2017

Rollup of 9 pull requests #38826

Closed

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Jan 4, 2017

Rollup merge of rust-lang#38581 - frewsxcv:vecdequeue-insert, r=Guill…

9226613

…aumeGomez Clarify behavior of `VecDeque::insert`. Fixes rust-lang#37046.

steveklabnik mentioned this pull request Jan 4, 2017

Rollup of 11 pull requests #38834

Closed

frewsxcv mentioned this pull request Jan 9, 2017

Rollup of 19 pull requests #38943

Closed

bors added a commit that referenced this pull request Jan 9, 2017

Auto merge of #38581 - frewsxcv:vecdequeue-insert, r=GuillaumeGomez

5c1472a

Clarify behavior of `VecDeque::insert`. Fixes #37046.

bors merged commit 0ab7812 into rust-lang:master Jan 9, 2017

bors mentioned this pull request Jan 9, 2017

Implement PartialEq<[A]> for VecDeque<A>. #38661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify behavior of `VecDeque::insert`. #38581

Clarify behavior of `VecDeque::insert`. #38581

frewsxcv commented Dec 23, 2016

rust-highfive commented Dec 23, 2016

frewsxcv commented Dec 23, 2016

Mark-Simulacrum Dec 23, 2016 •

edited

Loading

frewsxcv Dec 24, 2016

Mark-Simulacrum Dec 24, 2016

frewsxcv Dec 24, 2016

frewsxcv Dec 24, 2016

shepmaster commented Dec 23, 2016 •

edited

Loading

frewsxcv commented Dec 24, 2016

Mark-Simulacrum left a comment

Mark-Simulacrum Dec 24, 2016

frewsxcv Dec 25, 2016

Mark-Simulacrum Dec 24, 2016

frewsxcv Dec 25, 2016

Mark-Simulacrum Dec 25, 2016

frewsxcv Dec 26, 2016

GuillaumeGomez commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 9, 2017

bors commented Jan 9, 2017

alexcrichton commented Jan 9, 2017 via email

bors commented Jan 9, 2017

bors commented Jan 9, 2017

Clarify behavior of VecDeque::insert. #38581

Clarify behavior of VecDeque::insert. #38581

Conversation

frewsxcv commented Dec 23, 2016

rust-highfive commented Dec 23, 2016

frewsxcv commented Dec 23, 2016

Mark-Simulacrum Dec 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shepmaster commented Dec 23, 2016 • edited Loading

frewsxcv commented Dec 24, 2016

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 9, 2017

bors commented Jan 9, 2017

alexcrichton commented Jan 9, 2017 via email

bors commented Jan 9, 2017

bors commented Jan 9, 2017

Clarify behavior of `VecDeque::insert`. #38581

Clarify behavior of `VecDeque::insert`. #38581

Mark-Simulacrum Dec 23, 2016 •

edited

Loading

shepmaster commented Dec 23, 2016 •

edited

Loading