Skip to content

Rename IoSlice(Mut)::advance to advance_slice and add IoSlice(Mut)::advance - #85802

Merged
bors merged 4 commits into
rust-lang:masterfrom
Thomasdezeeuw:ioslice-advance
Jun 17, 2021
Merged

Rename IoSlice(Mut)::advance to advance_slice and add IoSlice(Mut)::advance#85802
bors merged 4 commits into
rust-lang:masterfrom
Thomasdezeeuw:ioslice-advance

Conversation

@Thomasdezeeuw

@ThomasdezeeuwThomasdezeeuw commented May 29, 2021

Copy link
Copy Markdown
Contributor

Also changes the signature of advance_slice to accept a &mut &mut [IoSlice], not returning anything. This will better match the IoSlice::advance function.

Updates #62726.

@Thomasdezeeuw
Rename IoSlice(Mut)::advance to advance_slice
3803c09
To make way for a new IoSlice(Mut)::advance function that advances a
single slice.
Also changes the signature to accept a `&mut &mut [IoSlice]`, not
returning anything. This will better match the future IoSlice::advance
function.
@Thomasdezeeuw
Add IoSlice(Mut)::advance
49e25b5
Advance the internal cursor of a single slice.
@rust-highfive

rust-highfive commented May 29, 2021

Copy link
Copy Markdown
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2021
@ThomasdezeeuwThomasdezeeuw mentioned this pull request May 29, 2021
Closed
3 tasks
@Mark-Simulacrum

Mark-Simulacrum commented May 29, 2021

Copy link
Copy Markdown
Member

r? @m-ou-se but looks okay to me at a cursory look (I have little context on these API changes though)

@Thomasdezeeuw

Thomasdezeeuw commented Jun 4, 2021

Copy link
Copy Markdown
ContributorAuthor

@Nemo157, @seanmonstar, @mpdn, @Ekleog you've shown interested in #62726, perhaps one of you can give a review?

seanmonstar
seanmonstar reviewed Jun 4, 2021
View reviewed changes
Comment threadlibrary/std/src/io/mod.rs Outdated
@Ekleog

Ekleog commented Jun 4, 2021

Copy link
Copy Markdown

@Thomasdezeeuw I'm sorry, but as of right now I don't really see the point in this API change, so I wouldn't be the right person to ask about that.

As far as I can tell, the slice.advance(n) function you add is basically slice = IoSlice::new(&slice[n..]), except in very specific situations, like the one that raises the need for the function you rename into advance_slices. Maybe giving a concrete example of a time you needed this new advance function in a way where storing an [IoSlice; 1] and using the array-based advance would be too boilerplate-y would help other people like me understand what problem this new function is trying to solve?

As for the &mut -> &mut vs. &mut &mut change, as per my stance in #62726 I can't think of any case where it would have a significant impact, so whichever gets us fastest to stabilization is good to me :)

@Thomasdezeeuw

Thomasdezeeuw commented Jun 5, 2021

Copy link
Copy Markdown
ContributorAuthor

@Thomasdezeeuw I'm sorry, but as of right now I don't really see the point in this API change, so I wouldn't be the right person to ask about that.

As far as I can tell, the slice.advance(n) function you add is basically slice = IoSlice::new(&slice[n..]), except in very specific situations, like the one that raises the need for the function you rename into advance_slices.

I don't remember the exact reason, but when using IoSliceMut at times this didn't work due to compiler not understand we're essentially reusing the lifetime. Then you needed to go through an awkward mem::replace(&mut buf, &[]) step. But like I said I don't quire remember the details.

Maybe giving a concrete example of a time you needed this new advance function in a way where storing an [IoSlice; 1] and using the array-based advance would be too boilerplate-y would help other people like me understand what problem this new function is trying to solve?

As for the &mut -> &mut vs. &mut &mut change, as per my stance in #62726 I can't think of any case where it would have a significant impact, so whichever gets us fastest to stabilization is good to me :)

@Ekleog

Ekleog commented Jun 5, 2021

Copy link
Copy Markdown

Is it possible that this memory is the one of implementing the function you're renaming into advance_slices, or are you sure you saw it happen in a different setup? Given it's exactly the way you did it the first time around, and is also the only instance I have personally met, I'm wondering if this vague recollection couldn't maybe match this. Especially as I can't think of a case where mem::replace(_, &mut []) would be useful from outside the stdlib on an IoSlice[Mut], as the inner slice isn't exposed.

Sorry if this question is completely wrong, it's totally possible I'm just failing to see the right use!

@Thomasdezeeuw

Thomasdezeeuw commented Jun 6, 2021

Copy link
Copy Markdown
ContributorAuthor

@Ekleog I'm not 100% sure anymore. There is https://github.com/rust-lang/futures-rs/blob/e21b5151a1dee1e53c792d2d5bee583633c6699d/futures-util/src/io/write_all_vectored.rs#L37, which uses mem::take (same as mem::replace(bufs, &mut [])), but that uses advance_slices (or would use).

I'm also fine with removing advance, keeping the advance_slices name in case we do ever want the single slice function.

@Thomasdezeeuw

Thomasdezeeuw commented Jun 10, 2021

Copy link
Copy Markdown
ContributorAuthor

@m-ou-se do you have time to review this, or should I try to assign someone else?

@m-ou-sem-ou-se added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` T-libs-api [DEPRECATED; DO NOT USE] labels Jun 17, 2021
m-ou-se
m-ou-se approved these changes Jun 17, 2021
View reviewed changes

@m-ou-sem-ou-se left a comment

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.

Sorry for the delay. This looks good to me! Just two typos:

Comment threadlibrary/std/src/io/mod.rs Outdated
Comment threadlibrary/std/src/io/mod.rs Outdated
@m-ou-se

m-ou-se commented Jun 17, 2021

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 17, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 5e7a8c6 has been approved by m-ou-se

@borsbors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2021
@JohnTitorJohnTitor mentioned this pull request Jun 17, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 17, 2021
@bors
Auto merge ofrust-lang#86399- JohnTitor:rollup-qlm2dvz, r=JohnTitor
4d3ce2e
Rollup of 7 pull requests
Successful merges:
- rust-lang#85663 (Document Arc::from)
- rust-lang#85802 (Rename IoSlice(Mut)::advance to advance_slice and add IoSlice(Mut)::advance)
- rust-lang#85970 (Remove methods under Implementors on trait pages)
- rust-lang#86340 (Use better error message for hard errors in CTFE)
- rust-lang#86343 (Do not emit invalid suggestions on multiple mutable borrow errors)
- rust-lang#86355 (Remove invalid suggestions for assoc consts on placeholder type error)
- rust-lang#86389 (Make `sum()` and `product()` documentation hyperlinks refer to `Iterator` methods.)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 31ee680 into rust-lang:masterJun 17, 2021
@rustbotrustbot added this to the 1.55.0 milestone Jun 17, 2021
@Thomasdezeeuw
Thomasdezeeuw deleted the ioslice-advance branch June 17, 2021 19:03
@Thomasdezeeuw

Thomasdezeeuw commented Jun 17, 2021

Copy link
Copy Markdown
ContributorAuthor

Thanks @m-ou-se

@taiki-etaiki-e mentioned this pull request Jun 18, 2021
Closed
github-actionsBot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
@JohnTitorgitbot
Rollup merge ofrust-lang#85802- Thomasdezeeuw:ioslice-advance, r=m-…
0a4352e
…ou-se
Rename IoSlice(Mut)::advance to advance_slice and add IoSlice(Mut)::advance
Also changes the signature of `advance_slice` to accept a `&mut &mut [IoSlice]`, not returning anything. This will better match the `IoSlice::advance` function.
Updates rust-lang#62726.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@m-ou-sem-ou-sem-ou-se approved these changes
+1 more reviewer
@seanmonstarseanmonstarseanmonstar left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@m-ou-sem-ou-se

Labels

A-ioArea: `std::io`, `std::fs`, `std::net` and `std::path`S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-libs-api[DEPRECATED; DO NOT USE]

Projects

None yet

Milestone

1.55.0

Development

Successfully merging this pull request may close these issues.

8 participants

@Thomasdezeeuw@rust-highfive@Mark-Simulacrum@Ekleog@m-ou-se@bors@seanmonstar@rustbot