Improve documentation of `Vec::split_off(...)` by mqudsi · Pull Request #65739 · rust-lang/rust

mqudsi · 2019-10-23T21:36:05Z

The previous ordering of the sentences kept switching between the return
value and the value of self after execution, making it hard to follow.

Additionally, as rendered in the browser, the period in "Self. self"
was difficult to make out as being a sentence separator and not one code
block.

rust-highfive · 2019-10-23T21:36:15Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (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.

dtolnay

Hmm, I find the original wording clearer here, although neither is great and I agree that the `Self`. `self` part comes out weird.

I think something like this would work best:

Returns a newly allocated vector containing elements [at, len) of self. The original vector will be left holding elements [0, at), with its capacity unchanged.

dtolnay · 2019-10-25T06:30:11Z

Note that these are moves not copies, as the Copy trait is not involved.

JohnCSimon · 2019-11-02T00:59:36Z

Ping from triage.
@mqudsi , please address the comments from @dtolnay
Thank you!

mqudsi · 2019-11-06T01:09:38Z

What would be the best way to make it clear that this is not a "cheap" operation (my intention behind the (incorrect) usage of "copies" in the text)?

Since the original vector retains its previous capacity (which necessarily overlaps in memory with the range being split off), an allocation at least equivalent to [at, len) must be made.

dtolnay · 2019-11-10T05:18:33Z

I think "newly allocated vector" in my recommended wording is sufficient to indicate that there is a cost. I don't know that the cost is worth emphasizing beyond that.

JohnCSimon · 2019-11-16T19:30:57Z

Ping from triage:
@dtolnay is this PR ready for review - can it be approved for merging?
cc: @mqudsi
Thanks

dtolnay · 2019-11-16T19:33:53Z

It looks like nothing has happened since my last review.

mqudsi · 2019-11-16T20:12:46Z

I'm sorry, I received the notifications about force pushes (presumably from rebasing) and thought that someone had taken over the issue.

I have pushed an update as requested.

The previous ordering of the sentences kept switching between the return value and the value of `self` after execution, making it hard to follow. Additionally, as rendered in the browser, the period in "`Self`. `self`" was difficult to make out as being a sentence separator and not one code block.

Remove the incorrect usage of "copy" as the trait is not called.

dtolnay · 2019-11-16T20:35:11Z

Sorry about the confusion -- @Dylan-DPC had pushed something to your branch, which I reverted. @Dylan-DPC please don't do that. It didn't even address the earlier review.

dtolnay

Thanks! I am happy with the new wording.

dtolnay · 2019-11-16T20:39:52Z

@bors r+

bors · 2019-11-16T20:39:54Z

📌 Commit a36c3f6 has been approved by dtolnay

mqudsi · 2019-11-16T20:42:42Z

Thanks for reviewing and for helping with the wording!

fwiw, I personally didn't like all the self and Self in the original wording (rather than using the word "vector") but assumed that it was there for a reason or by convention. Glad it's not the case ☺

ecstatic-morse · 2019-11-16T20:58:55Z

@bors rollup=always

Improve documentation of `Vec::split_off(...)` The previous ordering of the sentences kept switching between the return value and the value of `self` after execution, making it hard to follow. Additionally, as rendered in the browser, the period in "`Self`. `self`" was difficult to make out as being a sentence separator and not one code block.

@ghost

Rollup of 11 pull requests Successful merges: - #65739 (Improve documentation of `Vec::split_off(...)`) - #66271 (syntax: Keep string literals in ABIs and `asm!` more precisely) - #66344 (rustc_plugin: Remove `Registry::register_attribute`) - #66381 (find_deprecation: deprecation attr may be ill-formed meta.) - #66395 (Centralize panic macro documentation) - #66456 (Move `DIAGNOSTICS` usage to `rustc_driver`) - #66465 (add missing 'static lifetime in docs) - #66466 (miri panic_unwind: fix hack for SEH platforms) - #66469 (Use "field is never read" instead of "field is never used") - #66471 (Add test for issue 63116) - #66477 (Clarify transmute_copy documentation example) Failed merges: r? @ghost

rust-highfive assigned dtolnay Oct 23, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2019

dtolnay reviewed Oct 25, 2019
View reviewed changes

Comment threadsrc/liballoc/vec.rs Outdated
dtolnayOct 25, 2019
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that these are moves not copies, as the Copy trait is not involved.

dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2019

dtolnay force-pushed the vec_split_off_docs branch from 7300c78 to d900cc8Compare November 12, 2019 01:39

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 16, 2019

dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2019

mqudsi force-pushed the vec_split_off_docs branch from c5d5b2a to 5bbabe6Compare November 16, 2019 20:13

mqudsi added 2 commits November 16, 2019 14:15

Revise the text ofvec::split_off()per review inrust-lang#65739
a36c3f6
Remove the incorrect usage of "copy" as the trait is not called.

mqudsi force-pushed the vec_split_off_docs branch from 5bbabe6 to a36c3f6Compare November 16, 2019 20:15

dtolnay approved these changes Nov 16, 2019
View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 16, 2019

Centril mentioned this pull request Nov 17, 2019
Rollup of 11 pull requests #66483
Closed

JohnTitor mentioned this pull request Nov 17, 2019
Rollup of 11 pull requests #66485
Merged

bors merged commit a36c3f6 into rust-lang:masterNov 17, 2019

Uh oh!

Uh oh!

Conversation

mqudsi commented Oct 23, 2019

Uh oh!

rust-highfive commented Oct 23, 2019

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

dtolnayOct 25, 2019

Choose a reason for hiding this comment

Uh oh!

JohnCSimon commented Nov 2, 2019

Uh oh!

mqudsi commented Nov 6, 2019

Uh oh!

dtolnay commented Nov 10, 2019

Uh oh!

JohnCSimon commented Nov 16, 2019

Uh oh!

dtolnay commented Nov 16, 2019

Uh oh!

mqudsi commented Nov 16, 2019

Uh oh!

dtolnay commented Nov 16, 2019

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

dtolnay commented Nov 16, 2019

Uh oh!

bors commented Nov 16, 2019

Uh oh!

mqudsi commented Nov 16, 2019

Uh oh!

ecstatic-morse commented Nov 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants