Skip to content

Implement Seek::stream_position() for BufReader#74366

Merged
bors merged 1 commit intorust-lang:masterfrom
t-rapp:tr-bufreader-pos
Sep 7, 2020
Merged

Implement Seek::stream_position() for BufReader#74366
bors merged 1 commit intorust-lang:masterfrom
t-rapp:tr-bufreader-pos

Conversation

@t-rapp
Copy link
Contributor

@t-rapp t-rapp commented Jul 15, 2020

Optimization over BufReader::seek() for getting the current position without flushing the internal buffer.

Related to #31100. Based on the code in #70577.

@rust-highfive
Copy link
Contributor

rust-highfive commented Jul 15, 2020

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 15, 2020
@t-rapp t-rapp mentioned this pull request Jul 15, 2020
Closed
LukasKalbertodt
LukasKalbertodt reviewed Jul 16, 2020
View reviewed changes
@bors

This comment has been minimized.

Sign in to view
@t-rapp t-rapp requested a review from LukasKalbertodt August 3, 2020 07:59
LukasKalbertodt
LukasKalbertodt suggested changes Aug 8, 2020
View reviewed changes
Copy link
Contributor

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me! Only thing I noticed is that the subtraction might underflow if the inner reader has a buggy stream_position implementation. Not sure if we really need to harden against that. But I think I would prefer, for now, if you could replace the subtraction with:

pos.checked_sub(...).expect("buggy `stream_position` implementation in inner reader of BufReader")

Or something like that.

jyn514
jyn514 reviewed Aug 8, 2020
View reviewed changes
@LukasKalbertodt LukasKalbertodt 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 Aug 12, 2020
@t-rapp
Copy link
Contributor Author

t-rapp commented Aug 13, 2020

Updated the implementation to check for subtraction underflow / overflow. Realized that besides a buggy inner reader, this situation can also occur when misusing the (unfortunately public) BufReader::get_mut() method. Added a test for that case.

@kennytm
Copy link
Member

kennytm commented Aug 13, 2020

r? @LukasKalbertodt

jyn514
jyn514 reviewed Aug 13, 2020
View reviewed changes
LukasKalbertodt
LukasKalbertodt suggested changes Aug 25, 2020
View reviewed changes
Copy link
Contributor

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. But looks almost ready to merge now! Just a tiny thing I noticed.

@bors

This comment has been minimized.

Sign in to view
@jyn514 jyn514 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. 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 Aug 31, 2020
@bors

This comment has been minimized.

Sign in to view
LukasKalbertodt
LukasKalbertodt suggested changes Sep 5, 2020
View reviewed changes
Copy link
Contributor

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Oops, I found another minor thing °_°
Sorry for not noticing that in my last review.

@t-rapp
Implement Seek::stream_position() for BufReader
246d327 
Optimization over BufReader::seek() for getting the current position
without flushing the internal buffer.

Related to #31100. Based on code in #70577.
@LukasKalbertodt
Copy link
Contributor

LukasKalbertodt commented Sep 7, 2020

Looks good to me!

@bors r+

Thanks for sticking with this PR for so long! :)

@bors
Copy link
Collaborator

bors commented Sep 7, 2020

📌 Commit 246d327 has been approved by LukasKalbertodt

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2020
@bors
Copy link
Collaborator

bors commented Sep 7, 2020

⌛ Testing commit 246d327 with merge 9fe551a...

@bors
Copy link
Collaborator

bors commented Sep 7, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: LukasKalbertodt
Pushing 9fe551a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 7, 2020
@bors bors merged commit 9fe551a into rust-lang:master Sep 7, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 7, 2020
@t-rapp t-rapp deleted the tr-bufreader-pos branch March 25, 2021 13:45
@t-rapp t-rapp mentioned this pull request Apr 6, 2021
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@jyn514 jyn514 jyn514 left review comments

@LukasKalbertodt LukasKalbertodt LukasKalbertodt requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@LukasKalbertodt LukasKalbertodt

Labels

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-api Relevant to the library API team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.48.0

Development

Successfully merging this pull request may close these issues.

7 participants

@t-rapp @rust-highfive @bors @kennytm @LukasKalbertodt @jyn514 @rustbot