Implement Seek::stream_position() for BufReader by t-rapp · Pull Request #74366 · rust-lang/rust

t-rapp · 2020-07-15T14:29:48Z

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 · 2020-07-15T14:29:51Z

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

LukasKalbertodt

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.

t-rapp · 2020-08-13T07:17:28Z

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 · 2020-08-13T07:27:17Z

r? @LukasKalbertodt

LukasKalbertodt

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

LukasKalbertodt

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

Optimization over BufReader::seek() for getting the current position without flushing the internal buffer. Related to #31100. Based on code in #70577.

LukasKalbertodt · 2020-09-07T09:30:03Z

Looks good to me!

@bors r+

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

bors · 2020-09-07T09:30:04Z

📌 Commit 246d327 has been approved by LukasKalbertodt

bors · 2020-09-07T11:09:46Z

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

bors · 2020-09-07T13:29:37Z

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

rust-highfive assigned kennytm Jul 15, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 15, 2020

t-rapp mentioned this pull request Jul 15, 2020
Seeking from Current(0) return current stream position. #70577
Closed

LukasKalbertodt reviewed Jul 16, 2020
View reviewed changes

Comment threadsrc/libstd/io/buffered.rs Outdated

This comment has been minimized.
Sign in to view

t-rapp requested a review from LukasKalbertodt August 3, 2020 07:59

LukasKalbertodt suggested changes Aug 8, 2020
View reviewed changes

jyn514 reviewed Aug 8, 2020
View reviewed changes

Comment threadsrc/libstd/io/buffered.rs Outdated

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

rust-highfive assigned LukasKalbertodt and unassigned kennytm Aug 13, 2020

jyn514 reviewed Aug 13, 2020
View reviewed changes

Comment threadlibrary/std/src/io/buffered.rs Outdated
Comment threadlibrary/std/src/io/buffered.rs Outdated

LukasKalbertodt suggested changes Aug 25, 2020
View reviewed changes

Comment threadlibrary/std/src/io/buffered.rs Outdated

This comment has been minimized.
Sign in to view

jyn514 added T-libs-api [DEPRECATED; DO NOT USE] 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

This comment has been minimized.
Sign in to view

LukasKalbertodt suggested changes Sep 5, 2020
View reviewed changes

Comment threadlibrary/std/src/io/buffered/tests.rs Outdated
Comment threadlibrary/std/src/io/buffered/tests.rs Outdated

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.

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 added the merged-by-bors This PR was explicitly merged by bors. label Sep 7, 2020

bors merged commit 9fe551a into rust-lang:masterSep 7, 2020

rustbot added this to the 1.48.0 milestone Sep 7, 2020

t-rapp deleted the tr-bufreader-pos branch March 25, 2021 13:45

t-rapp mentioned this pull request Apr 6, 2021
Tracking issue for Seek::{stream_len, stream_position} (feature seek_convenience) #59359
Open
2 tasks

Uh oh!

Uh oh!

Conversation

t-rapp commented Jul 15, 2020

Uh oh!

rust-highfive commented Jul 15, 2020

Uh oh!

Uh oh!

This comment has been minimized.

LukasKalbertodt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t-rapp commented Aug 13, 2020

Uh oh!

kennytm commented Aug 13, 2020

Uh oh!

Uh oh!

Uh oh!

LukasKalbertodt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

LukasKalbertodt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LukasKalbertodt commented Sep 7, 2020

Uh oh!

bors commented Sep 7, 2020

Uh oh!

bors commented Sep 7, 2020

Uh oh!

bors commented Sep 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants