Add MAIN_SEPARATOR_STR by SUPERCILEX · Pull Request #93976 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

SUPERCILEX

Currently, if someone needs access to the path separator as a str, they need to go through this mess:

unsafe { std::str::from_utf8_unchecked(slice::from_ref(&(MAIN_SEPARATOR as u8))) }

This PR just re-exports an existing path separator str API.

@rust-highfive

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

@Mark-Simulacrum

Just as a note, the user could use safe code to do the conversion -- std::path::MAIN_SEPARATOR.encode_utf8(&mut [0; 4]) returns &mut str -- but of course at the cost of a non-'static str. (And even that limitation will go away if encode_utf8 is made const, I believe).

Do you have an example use case where the &str is required and char doesn't work?

@SUPERCILEX

If encode_utf8 becomes const, I guess that would be better, but you still need to pass in a correctly sized buffer which is annoying. I've wanted to use it with an OsString:

    let mut s: OsString = ...
    s.push(unsafe {
        std::str::from_utf8_unchecked(slice::from_ref(&(MAIN_SEPARATOR as u8)))
    });

Is there a better way?

@Mark-Simulacrum

A 4-byte buffer will always work, so there's no need to really think that much about it. Your code will likely work if the unsafe block is replaced with std::path::MAIN_SEPARATOR.encode_utf8(&mut [0; 4]) as I suggest -- which doesn't really seem that bad to me.

If the specific goal is pushing into OsString, then I would personally lean towards a push_ch method (unfortunately OsString::push was not named push_str, like the similar method on String). This PR's solution might be simpler, but it feels a little unfortunate to me to have a MAIN_SEPARATOR and MAIN_SEPARATOR_STR side-by-side.

@SUPERCILEX

I guess we could add a new method to OsString, but str seems like the better API to me. If anything, I feel like MAIN_SEPARATOR should have been a str since there are basically no APIs that use char.

@Mark-Simulacrum

r? @yaahc for T-libs-api (as new unstable surface area)

I'm a little uncertain -- part of me wants to say the encode_utf8 is simple enough that we shouldn't add this -- but on the other hand, it's a pretty simple addition.

@the8472 the8472 added the T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.

label

Feb 14, 2022

@yaahc

I guess we could add a new method to OsString, but str seems like the better API to me. If anything, I feel like MAIN_SEPARATOR should have been a str since there are basically no APIs that use char.

A quick search of usages of MAIN_SEPARATOR show that it's already incredibly common for people to use it as &MAIN_SEPARATOR.to_string(), which strongly supports your assertion that it should be available as a &str.

I agree with @Mark-Simulacrum that having two copies of the same const side by side for diff types is unfortunate, but given that the existing one is already stable I'm not really sure how many good options we have. My immediate suspicion is that MAIN_SEPARATOR should have had it's own opaque type with methods or trait impls for exposing the separator as char or &str or whatever you need, but that ship has already sailed. I think I'm fine with merging this on nightly, then we can leave it solving the bigger questions of if this is the ideal solution to the whole team during stabilization.

@SUPERCILEX could you go ahead and create a tracking issue for the new feature you introduced?

@SUPERCILEX

@rust-log-analyzer

This comment has been minimized.

@SUPERCILEX

Signed-off-by: Alex Saveau saveau.alexandre@gmail.com

@yaahc

Awesome, thank you!

@bors r+

@bors

📌 Commit 80fde23 has been approved by yaahc

@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, 2022

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 17, 2022

@bors

…askrgr

Rollup of 9 pull requests

Successful merges:

Failed merges:

r? @ghost @rustbot modify labels: rollup

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.