Skip to content

Delegate to inner vec::IntoIter from env::ArgsOs #139847

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

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

Conversation

thaliaarchi
Copy link
Contributor

@thaliaarchi thaliaarchi commented Apr 15, 2025

Delegate from std::env::ArgsOs to the methods of the inner platform-specific iterators, when it would be more efficient than just using the default methods of its own impl. Most platforms use vec::IntoIter as the inner type, so prioritize delegating to the methods it provides.

std::env::Args is implemented atop std::env::ArgsOs and performs UTF-8 validation with a panic for invalid data. This is a visible effect which users certainly rely on, so we can't skip any arguments. Any further iterator methods would skip some elements, so no change is needed for that type.

Add #[inline] for any methods which simply wrap the inner iterator.

@rustbot
Copy link
Collaborator

rustbot commented Apr 15, 2025

r? @workingjubilee

rustbot has assigned @workingjubilee.
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 Apr 15, 2025
@thaliaarchi thaliaarchi force-pushed the args/delegate-iter branch 2 times, most recently from 3a60980 to b63849b Compare April 15, 2025 06:08
thaliaarchi added a commit to thaliaarchi/rust that referenced this pull request Apr 15, 2025
The zkVM implementation of `env::ArgsOs` reports the full length even
after having iterated once, which is incorrect. Instead, use a double
cursor approach which works out to be simpler. Also, implement more
iterator methods like the other platforms in rust-lang#139847.
thaliaarchi added a commit to thaliaarchi/rust that referenced this pull request Apr 15, 2025
The zkVM implementation of `env::ArgsOs` incorrectly reports the full
length even after having iterated. Instead, use a range approach which
works out to be simpler. Also, implement more iterator methods like the
other platforms in rust-lang#139847.
thaliaarchi added a commit to thaliaarchi/rust that referenced this pull request Apr 15, 2025
The zkVM implementation of `env::ArgsOs` incorrectly reports the full
length even after having iterated. Instead, use a range approach which
works out to be simpler. Also, implement more iterator methods like the
other platforms in rust-lang#139847.
@bors
Copy link
Collaborator

bors commented Apr 25, 2025

☔ The latest upstream changes (presumably #140282) made this pull request unmergeable. Please resolve the merge conflicts.

@workingjubilee
Copy link
Member

Since env::ArgsOs performs UTF-8 validation, it cannot benefit from this,

uhhh. hm.

I am slightly confused as to how to read the pull request's description, as my first attempt to understand it leads to deriving "the changes in this PR cannot benefit env::ArgsOs", which raises the question of why we are implementing these changes. can you clarify?

@workingjubilee
Copy link
Member

the code seems cromulent besides that, but I'll take a closer look with an explanation for what I should be looking for, because I feel like I missed a beat (lose the rhythm, and nothing falls into place! only missed by a fraction, slipped a little off your pace! oh!)

@rustbot author

@rustbot rustbot 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 May 1, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 1, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@workingjubilee
Copy link
Member

ah, the concern is with a third type! thank you, that is much clearer.

@thaliaarchi thaliaarchi force-pushed the args/delegate-iter branch 2 times, most recently from 90b027d to ac00d49 Compare May 1, 2025 06:30
@thaliaarchi
Copy link
Contributor Author

I've rebased to resolve merge conflicts and touched up a comment, separately, so it's sane to compare with this terrible UI.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

one nit, feel free to revise my suggestion as you see fit as long as it states this is a current understanding or "quality of implementation" detail and it is not completely clear that our contract hard-and-fast promises anything here.

@thaliaarchi thaliaarchi force-pushed the args/delegate-iter branch from ac00d49 to def3a30 Compare May 1, 2025 22:16
Delegate from `std::env::ArgsOs` to the methods of the inner
platform-specific iterators, when it would be more efficient than just
using the default methods of its own impl. Most platforms use
`vec::IntoIter` as the inner type, so prioritize delegating to the
methods it provides.

`std::env::Args` is implemented atop `std::env::ArgsOs` and performs
UTF-8 validation with a panic for invalid data. This is a visible effect
which users certainly rely on, so we can't skip any arguments. Any
further iterator methods would skip some elements, so no change is
needed for that type.

Add `#[inline]` for any methods which simply wrap the inner iterator.
@workingjubilee
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented May 2, 2025

📌 Commit 28deaa6 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 2, 2025
@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 May 2, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request May 2, 2025
…workingjubilee

Delegate to inner `vec::IntoIter` from `env::ArgsOs`

Delegate from `std::env::ArgsOs` to the methods of the inner platform-specific iterators, when it would be more efficient than just using the default methods of its own impl. Most platforms use `vec::IntoIter` as the inner type, so prioritize delegating to the methods it provides.

`std::env::Args` is implemented atop `std::env::ArgsOs` and performs UTF-8 validation with a panic for invalid data. This is a visible effect which users certainly rely on, so we can't skip any arguments. Any further iterator methods would skip some elements, so no change is needed for that type.

Add `#[inline]` for any methods which simply wrap the inner iterator.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2025
Rollup of 12 pull requests

Successful merges:

 - rust-lang#134034 (handle paren in macro expand for let-init-else expr)
 - rust-lang#137474 (pretty-print: Print shebang at the top of the output)
 - rust-lang#138872 (rustc_target: RISC-V `Zfinx` is incompatible with `{ILP32,LP64}[FD]` ABIs)
 - rust-lang#139046 (Improve `Lifetime::suggestion`)
 - rust-lang#139206 (std: use the address of `errno` to identify threads in `unique_thread_exit`)
 - rust-lang#139608 (Clarify `async` block behaviour)
 - rust-lang#139847 (Delegate to inner `vec::IntoIter` from `env::ArgsOs`)
 - rust-lang#140159 (Avoid redundant WTF-8 checks in `PathBuf`)
 - rust-lang#140197 (Document breaking out of a named code block)
 - rust-lang#140389 (Remove `avx512dq` and `avx512vl` implication for `avx512fp16`)
 - rust-lang#140430 (Improve test coverage of HIR pretty printing.)
 - rust-lang#140507 (rustc_target: RISC-V: feature addition batch 3)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

4 participants