Skip to content

Don't panic in <BorrowedCursor as io::Write>::write #115460

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.

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

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Sep 2, 2023

Instead of panicking if the BorrowedCursor does not have enough capacity for the whole buffer, just return a short write, like <&mut [u8] as io::Write>::write does.

(cc @ChayimFriedman2 #78485 (comment))

(I'm not sure if this needs an ACP? since it's not changing the "API", just what the function does)

@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@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 Sep 2, 2023
@zachs18 zachs18 marked this pull request as ready for review September 2, 2023 02:42
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

This makes sense to me!

@djc
Copy link
Contributor

djc commented Sep 20, 2023

@thomcc gentle ping? Should this get another reviewer?

@joboet
Copy link
Member

joboet commented Oct 21, 2023

I'm going to take the liberty of assigning this to someone from libs-api since this is technically a user-facing change, even if it merely fixes a bug.

r? libs-api
@rustbot label +T-libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 21, 2023
@rustbot rustbot assigned Amanieu and unassigned thomcc Oct 21, 2023
@Amanieu
Copy link
Member

Amanieu commented Oct 27, 2023

This is a very minor API change, but ultimately I think the new behavior is better than the old one. It is also consistent with other Write implementations.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 27, 2023

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 27, 2023
@thomcc
Copy link
Member

thomcc commented Oct 27, 2023

Should libs be on this FCP?

@Amanieu
Copy link
Member

Amanieu commented Oct 27, 2023

Hmm probably not but at this point I don't think it really hurts.

@the8472
Copy link
Member

the8472 commented Oct 27, 2023

BorrowedCursor is unstable anyway and write_all turns this into an Errorkind::WriteZero. Seems fine.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 28, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Oct 28, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 7, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 7, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@dtolnay
Copy link
Member

dtolnay commented Nov 7, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 7, 2023

📌 Commit 11a64a1 has been approved by dtolnay

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 7, 2023
@dtolnay dtolnay assigned dtolnay and unassigned Amanieu Nov 8, 2023
@bors
Copy link
Collaborator

bors commented Nov 8, 2023

⌛ Testing commit 11a64a1 with merge 28acba3...

@bors
Copy link
Collaborator

bors commented Nov 8, 2023

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 28acba3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 8, 2023
@bors bors merged commit 28acba3 into rust-lang:master Nov 8, 2023
@rustbot rustbot added this to the 1.75.0 milestone Nov 8, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (28acba3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.9% [0.5%, 1.4%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.7% [-0.9%, -0.4%] 4
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.1% [-0.9%, 1.4%] 6

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 663.194s -> 662.725s (-0.07%)
Artifact size: 308.72 MiB -> 308.73 MiB (0.00%)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.