Improve documentation of Vec::split_off(...)
#65739
Conversation
|
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. |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
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)ofself. The original vector will be left holding elements[0, at), with its capacity unchanged.
src/liballoc/vec.rs
Outdated
| /// and the returned `Self` contains elements `[at, len)`. | ||
| /// | ||
| /// Note that the capacity of `self` does not change. | ||
| /// Copies the range from `[at, len)` to a newly allocated `Self` |
There was a problem hiding this comment.
Note that these are moves not copies, as the Copy trait is not involved.
dtolnay
added
S-waiting-on-author
|
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 |
|
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. |
dtolnay
force-pushed
the
vec_split_off_docs
branch
from
7300c78
to
d900cc8
Compare
JohnCSimon
added
S-waiting-on-review
dtolnay
added
S-waiting-on-author
|
It looks like nothing has happened since my last review. |
|
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. |
mqudsi
force-pushed
the
vec_split_off_docs
branch
from
c5d5b2a
to
5bbabe6
Compare
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.
mqudsi
force-pushed
the
vec_split_off_docs
branch
from
5bbabe6
to
a36c3f6
Compare
|
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. |
|
@bors r+ |
|
📌 Commit a36c3f6 has been approved by |
bors
added
S-waiting-on-bors
|
Thanks for reviewing and for helping with the wording! fwiw, I personally didn't like all the |
|
@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.
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.
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
The previous ordering of the sentences kept switching between the return
value and the value of
selfafter 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.