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.

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

Move OsStringExt and OsStrExt to std::os #84967

Merged
merged 3 commits into from
Jun 20, 2021
Merged

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented May 5, 2021

Moves the OsStringExt and OsStrExt traits and implementations from sys_common to os. sys_common is for abstractions over sys and shouldn't really contain publicly exported items.

This does introduce some duplication: the traits and implementations are now duplicated in unix, wasi, hermit, and sgx. However, I would argue that this duplication is no different to how something like MetadataExt is duplicated in linux, vxworkx, redox, solaris etc. The duplication also matches the fact that the traits on different platforms are technically distinct types: any platform is free to add it's own extra methods to the extension trait.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 May 5, 2021
@@ -34,5 +34,68 @@

#![unstable(feature = "sgx_platform", issue = "56975")]

#[unstable(feature = "sgx_platform", issue = "56975")]

This comment was marked as outdated.

@CDirkx

This comment has been minimized.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 6, 2021
Comment on lines -22 to +23
// FIXME:
// `Buf::as_slice` current implementation relies
// on `Slice` being layout-compatible with `[u8]`.
// When attribute privacy is implemented, `Slice` should be annotated as `#[repr(transparent)]`.
// Anyway, `Slice` representation and layout are considered implementation detail, are
// not documented and must not be relied upon.
pub(crate) struct Slice {
#[repr(transparent)]
pub struct Slice {
Copy link
Contributor Author

@CDirkx CDirkx May 13, 2021

Choose a reason for hiding this comment

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

Because Buf and Slice now no longer live in a publicly exported module they can be marked as pub and repr(transparent) as normal.

@bors
Copy link
Contributor

bors commented May 20, 2021

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

@m-ou-se
Copy link
Member

m-ou-se commented Jun 5, 2021

I don't think we should be duplicating this. Duplication in std/src/sys has often led to badly maintained code in the past, where only some copies were updated. (E.g. documentation patches, etc.)

It might be good to have each platform opt-in to these extensions instead of just using not(windows) in sys_common/mod like it is now, but if we do that, we shouldn't be duplicating so much code and documentation.

@m-ou-se m-ou-se 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 Jun 5, 2021
@CDirkx
Copy link
Contributor Author

CDirkx commented Jun 20, 2021

I tried another approach of reusing code within std::os using #[path], like is done in sys. This removes the earlier duplication while moving OsStrExt/OsStringExt to the (in my opinion) more appropriate location with all the other extension traits.

One potential problem I could think of is that it is more easy to forget that a file is reused on other platforms, more so than in sys. A unix specific change to for example documentation would be incorrect on other platforms. Because of this I added a comment explicitly stating that the file is reused.

What are your thoughts on this approach? If you approve I could later look into if there are more files in std::os that can also be deduplicated this way.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 20, 2021

Thanks!

This looks very reasonable to me.

We could also have a os::common module or something for things used by multiple platforms, but in many ways, os::unix is that already.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 20, 2021

📌 Commit ad7b897 has been approved by m-ou-se

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 20, 2021
@bors
Copy link
Contributor

bors commented Jun 20, 2021

⌛ Testing commit ad7b897 with merge dd94145...

@bors
Copy link
Contributor

bors commented Jun 20, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing dd94145 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 20, 2021
@bors bors merged commit dd94145 into rust-lang:master Jun 20, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 20, 2021
@CDirkx CDirkx deleted the os_str_ext branch June 21, 2021 09:47
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 2, 2021
Move `os_str_bytes` to `sys::unix`

Followup to rust-lang#84967, with `OsStrExt` and `OsStringExt` moved out of `sys_common`, there is no reason anymore for `os_str_bytes` to live in `sys_common` and not in sys. This pr moves it to the location `sys::unix::os_str` and reuses the code on other platforms via `#[path]` (as is common in `sys`) instead of importing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 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.

5 participants