Skip to content
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

Use tell for <File as Seek>::stream_position #137165

Merged
merged 1 commit into from
Feb 18, 2025
Merged

Use tell for <File as Seek>::stream_position #137165

merged 1 commit into from
Feb 18, 2025

Conversation

thaliaarchi
Copy link
Contributor

@thaliaarchi thaliaarchi commented Feb 17, 2025 •
edited
Loading

Some platforms have a more efficient way to get the current offset of the file than by seeking. For example, Wasi has fd_tell and SOLID has SOLID_FS_Ftell. Implement <File as Seek>::stream_position() in terms of those.

I do not use any APIs that were not already used in std. Although, the libc crate has ftell, ftello, and ftello64, I do not know platform coverage. It appears that Windows has no tell-like API.

I have checked that it builds on each relevant platform.

@rustbot
Copy link
Collaborator

rustbot commented Feb 17, 2025

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
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 O-hermit Operating System: Hermit O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows 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 Feb 17, 2025
@thaliaarchi thaliaarchi mentioned this pull request Feb 17, 2025
Closed
@ChrisDenton
Copy link
Member

For platforms that don't have a tell function, I'd rather they be implemented in terms of self.seek rather than duplicating the code.

@ChrisDenton
Copy link
Member

Also cc @kawadakk for changes to SOLID.

@thaliaarchi
Copy link
Contributor Author

thaliaarchi commented Feb 17, 2025 •
edited
Loading

For platforms that don't have a tell function, I'd rather they be implemented in terms of self.seek rather than duplicating the code.

I was thinking that eliminating the match would be good, since these aren't(?) inlined, but that's also a good approach. Updated.

@thaliaarchi
Copy link
Contributor Author

Another approach could be to impl Seek on each of the pal File types so they can implement as much as they want. I think that could be fitting for Read/Write as well (separately).

@ChrisDenton
Copy link
Member

I think this is good for now. I do agree that implementing the trait may ultimately be better but, as you might be aware, std/sys is undergoing a reorganisation at the moment so I'd prefer to wait until there's a std/sys/fs module before doing bigger changes.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 17, 2025

📌 Commit 7112474 has been approved by ChrisDenton

It is now in the queue for this repository.

@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 Feb 17, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 17, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2025
@bors
Auto merge of rust-lang#137176 - matthiaskrgr:rollup-eht05gr, r=matth…
de91711 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#136959 (Simplify switch sources)
 - rust-lang#137020 (Pass vendored sources from bootstrap to generate-copyright)
 - rust-lang#137073 (boostrap: skip no_std targets in Std doc step)
 - rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`)
 - rust-lang#137166 (Update default loongarch code model in docs)
 - rust-lang#137168 (correct comment)
 - rust-lang#137169 (CI: rfl: move job forward to Linux v6.14-rc3)
 - rust-lang#137170 (Allow configuring jemalloc per target)
 - rust-lang#137173 (Subtree update of `rust-analyzer`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 10018d8 into rust-lang:master Feb 18, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 18, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2025
@rust-timer
Unrolled build for rust-lang#137165
446c046 
Rollup merge of rust-lang#137165 - thaliaarchi:file-tell, r=ChrisDenton

Use `tell` for `<File as Seek>::stream_position`

Some platforms have a more efficient way to get the current offset of the file than by seeking. For example, Wasi has `fd_tell` and SOLID has `SOLID_FS_Ftell`. Implement `<File as Seek>::stream_position()` in terms of those.

I do not use any APIs that were not already used in `std`. Although, the `libc` crate has [`ftell`](https://docs.rs/libc/latest/libc/fn.ftell.html), [`ftello`](https://docs.rs/libc/latest/libc/fn.ftello.html), and [`ftello64`](https://docs.rs/libc/latest/libc/fn.ftello64.html), I do not know platform coverage. It appears that Windows has no `tell`-like API.

I have checked that it builds on each relevant platform.
@thaliaarchi thaliaarchi deleted the file-tell branch February 18, 2025 04:03
thaliaarchi added a commit to thaliaarchi/rust that referenced this pull request Feb 18, 2025
@thaliaarchi
Remove std::os::wasi::fs::FileExt::tell
d2f1597 
Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`),
`tell` is now directly exposed via `stream_position`, making
`<File as FileExt>::tell` redundant. Remove it.
@thaliaarchi thaliaarchi mentioned this pull request Feb 18, 2025
Merged
@thaliaarchi thaliaarchi mentioned this pull request Feb 18, 2025
Open
2 tasks
Urgau added a commit to Urgau/rust that referenced this pull request Feb 18, 2025
@Urgau
Rollup merge of rust-lang#137205 - thaliaarchi:remove-wasi-fileext-te…
5063673 
…ll, r=alexcrichton

Remove `std::os::wasi::fs::FileExt::tell`

Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`), `tell` is now directly exposed via `stream_position`, making `<File as FileExt>::tell` redundant. Remove it.

`std::os::wasi::fs::FileExt::tell` is currently unstable and tracked in rust-lang#71213.

`@rustbot` ping wasi
Urgau added a commit to Urgau/rust that referenced this pull request Feb 18, 2025
@Urgau
Rollup merge of rust-lang#137205 - thaliaarchi:remove-wasi-fileext-te…
73e5abd 
…ll, r=alexcrichton

Remove `std::os::wasi::fs::FileExt::tell`

Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`), `tell` is now directly exposed via `stream_position`, making `<File as FileExt>::tell` redundant. Remove it.

`std::os::wasi::fs::FileExt::tell` is currently unstable and tracked in rust-lang#71213.

``@rustbot`` ping wasi
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2025
@rust-timer
Unrolled build for rust-lang#137205
5c2a6bf 
Rollup merge of rust-lang#137205 - thaliaarchi:remove-wasi-fileext-tell, r=alexcrichton

Remove `std::os::wasi::fs::FileExt::tell`

Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`), `tell` is now directly exposed via `stream_position`, making `<File as FileExt>::tell` redundant. Remove it.

`std::os::wasi::fs::FileExt::tell` is currently unstable and tracked in rust-lang#71213.

``@rustbot`` ping wasi
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Feb 22, 2025
@thaliaarchi
Remove std::os::wasi::fs::FileExt::tell
8291ead 
Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`),
`tell` is now directly exposed via `stream_position`, making
`<File as FileExt>::tell` redundant. Remove it.
github-actions bot pushed a commit to carolynzech/rust that referenced this pull request Feb 22, 2025
@thaliaarchi
Remove std::os::wasi::fs::FileExt::tell
82860bb 
Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`),
`tell` is now directly exposed via `stream_position`, making
`<File as FileExt>::tell` redundant. Remove it.
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Feb 22, 2025
@thaliaarchi
Remove std::os::wasi::fs::FileExt::tell
9fe471c 
Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`),
`tell` is now directly exposed via `stream_position`, making
`<File as FileExt>::tell` redundant. Remove it.
github-actions bot pushed a commit to thanhnguyen-aws/verify-rust-std that referenced this pull request Feb 22, 2025
@thaliaarchi
Remove std::os::wasi::fs::FileExt::tell
16a1a0e 
Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`),
`tell` is now directly exposed via `stream_position`, making
`<File as FileExt>::tell` redundant. Remove it.
github-actions bot pushed a commit to thanhnguyen-aws/verify-rust-std that referenced this pull request Mar 3, 2025
@thaliaarchi
Remove std::os::wasi::fs::FileExt::tell
e36d971 
Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`),
`tell` is now directly exposed via `stream_position`, making
`<File as FileExt>::tell` redundant. Remove it.
github-actions bot pushed a commit to thanhnguyen-aws/verify-rust-std that referenced this pull request Mar 4, 2025
@thaliaarchi
Remove std::os::wasi::fs::FileExt::tell
fbe3ccc 
Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`),
`tell` is now directly exposed via `stream_position`, making
`<File as FileExt>::tell` redundant. Remove it.
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 6, 2025
@thaliaarchi
Remove std::os::wasi::fs::FileExt::tell
db00c40 
Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`),
`tell` is now directly exposed via `stream_position`, making
`<File as FileExt>::tell` redundant. Remove it.
github-actions bot pushed a commit to thanhnguyen-aws/verify-rust-std that referenced this pull request Mar 6, 2025
@thaliaarchi
Remove std::os::wasi::fs::FileExt::tell
cdbfcdc 
Following rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`),
`tell` is now directly exposed via `stream_position`, making
`<File as FileExt>::tell` redundant. Remove it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ChrisDenton ChrisDenton

Labels
O-hermit Operating System: Hermit O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows 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.
Projects
None yet
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
4 participants
@thaliaarchi @rustbot @ChrisDenton @bors