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

Extract sys::args::Args implementation to sys_common #84503

Closed
wants to merge 2 commits into from
Closed

Extract sys::args::Args implementation to sys_common #84503

wants to merge 2 commits into from

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Apr 24, 2021 •
edited
Loading

After #84413 and #84179 I have been looking into if code for sys::args::Args could be de-duplicated, as it was essentially the same on every platform: a wrapper type implementing !Send + !Sync + Debug + Iterator + ExactIterator + DoubleEndedIterator (although note that on windows and sgx the types are currently not !Send + !Sync).

This PR makes sys::args::Args a type alias (for vec::IntoIter/Empty/Clone<Iter<'static>> depending on the platform) and extracts the wrapper type to sys_common.

Alternatively, the wrapper type could be removed entirely and the type aliases used directly (i.e. only the first commit of this PR), since they are wrapped by std::env::ArgsOs anyway. These type aliases are not !Send + !Sync, but I don't know how much of a problem that would be since ArgsOs implements !Send + !Sync explicitly and currently sys::windows::args::Args and sys::sgx::args::Args are already Send and Sync.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 Apr 24, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 24, 2021 •
edited
Loading

One thing this is still blocked on is making sure the Debug output of std::env::Args and std::env::ArgsOs are not affected.

Edit: this has been resolved by relying on AsRef<[OsString]>, which in practice means that every sys::args::Args is now a type alias for vec::IntoIter.

@CDirkx CDirkx changed the title Args Extract sys::args::Args implementation to sys_common Apr 24, 2021
@CDirkx

This comment has been minimized.

Sign in to view
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 24, 2021
@bors
Copy link
Contributor

bors commented Apr 24, 2021

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

@bors
Copy link
Contributor

bors commented Apr 25, 2021

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

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 25, 2021

warning: spurious network error (1 tries remaining): failed to get 200 response from https://crates.io/api/v1/crates/once_cell/1.7.2/download, got 503
error: failed to download from https://crates.io/api/v1/crates/difference/2.0.0/download

@bors
Copy link
Contributor

bors commented May 7, 2021

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

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2021
@bors
Copy link
Contributor

bors commented Jun 20, 2021

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

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@bors
Copy link
Contributor

bors commented Sep 2, 2021

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

@JohnCSimon
Copy link
Member

Ping from triage:
@CDirkx can you please resolve the merge conflicts?

@joshtriplett
Copy link
Member

Seems reasonable to me; r=me with conflicts addressed.

@joshtriplett joshtriplett 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 Oct 4, 2021
@JohnCSimon
Copy link
Member

Ping again from triage:
@CDirkx can you please resolve the merge conflicts and after that, please set the tag to S-waiting-on-review thank you.

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 8, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 5, 2021
@joelpalmer joelpalmer 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2022
@Dylan-DPC
Copy link
Member

Dylan-DPC commented Feb 19, 2022 •
edited
Loading

Closing this as inactive

@Dylan-DPC Dylan-DPC closed this Feb 19, 2022
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@joshtriplett joshtriplett

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@CDirkx @rust-highfive @rust-log-analyzer @bors @JohnCSimon @joshtriplett @Dylan-DPC @joelpalmer @rustbot