Skip to content

core: Add BorrowedCursor::with_unfilled_buf #142885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

core: Add BorrowedCursor::with_unfilled_buf #142885

wants to merge 1 commit into from
+87 −0

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented Jun 22, 2025 •
edited by rustbot
Loading

Implementation of rust-lang/libs-team#367.

This mainly adds BorrowedCursor::with_unfilled_buf, with enables using the unfilled part of a cursor as a BorrowedBuf.

Note that unlike the ACP, BorrowedCursor::unfilled_buf was moved to a From conversion. This is more consistent with other ways of creating a BorrowedBuf and hides a bit this conversion that requires unsafe code to be used correctly.

Cc #78485 #117693
@rustbot
Copy link
Collaborator

rustbot commented Jun 22, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 22, 2025
Mark-Simulacrum
Mark-Simulacrum reviewed Jun 28, 2025
View reviewed changes
library/core/src/io/borrowed_buf.rs
let res = f(&mut buf);

// Check that the caller didn't replace the `BorrowedBuf`.
assert!(core::ptr::addr_eq(prev_ptr, buf.buf));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a necessary condition for soundness? If so, we probably have to abort here, not just panic. Can you clarify in comments why it's necessary to check this as well?

(Also if it's a soundness condition it should probably be in a drop guard, so that the closure panicking after replacing the Buf doesn't cause us to unwind past this check).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a necessary condition for soundness? If so, we probably have to abort here, not just panic. Can you clarify in comments why it's necessary to check this as well?

Yes, for the update of filled and init fields. I will change the comments to make it clearer.

I disagree for the abort though. It is not something usually done in std unless necessary, which is not the case here.

(Also if it's a soundness condition it should probably be in a drop guard, so that the closure panicking after replacing the Buf doesn't cause us to unwind past this check).

That's fine if we do, because the code that rely on this invariant would be skipped.
@a1phyr
core: Add BorrowedCursor::with_unfilled_buf
136d24f 
Implementation of rust-lang/libs-team#367.

This mainly adds `BorrowedCursor::with_unfilled_buf`, with enables using
the unfilled part of a cursor as a `BorrowedBuf`.

Note that unlike the ACP, `BorrowedCursor::unfilled_buf` was moved to a
`From` conversion. This is more consistent with other ways of creating a
`BorrowedBuf` and hides a bit this conversion that requires unsafe code
to be used correctly.
@a1phyr a1phyr force-pushed the borrowed_cursor_to_buf branch from 1b4a047 to 136d24f Compare July 1, 2025 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
3 participants
@a1phyr @rustbot @Mark-Simulacrum