-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add MAIN_SEPARATOR_STR #93976
Add MAIN_SEPARATOR_STR #93976
Conversation
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. |
Just as a note, the user could use safe code to do the conversion -- Do you have an example use case where the &str is required and char doesn't work? |
If let mut s: OsString = ...
s.push(unsafe {
std::str::from_utf8_unchecked(slice::from_ref(&(MAIN_SEPARATOR as u8)))
}); Is there a better way? |
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 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. |
I guess we could add a new method to OsString, but str seems like the better API to me. If anything, I feel like |
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. |
A quick search of usages of 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 @SUPERCILEX could you go ahead and create a tracking issue for the new feature you introduced? |
144845f
to
8181dd0
Compare
@yaahc Awesome, done! |
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
8181dd0
to
80fde23
Compare
Awesome, thank you! @bors r+ |
📌 Commit 80fde23 has been approved by |
…askrgr Rollup of 9 pull requests Successful merges: - rust-lang#93337 (Update tracking issue numbers for inline assembly sub-features) - rust-lang#93758 (Improve comments about type folding/visiting.) - rust-lang#93780 (Generate list instead of div items in sidebar) - rust-lang#93976 (Add MAIN_SEPARATOR_STR) - rust-lang#94011 (Even more let_else adoptions) - rust-lang#94041 (Add a `try_collect()` helper method to `Iterator`) - rust-lang#94043 (Fix ICE when using Box<T, A> with pointer sized A) - rust-lang#94082 (Remove CFG_PLATFORM) - rust-lang#94085 (Clippy: Don't lint `needless_borrow` in method receiver positions) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Currently, if someone needs access to the path separator as a str, they need to go through this mess:
This PR just re-exports an existing path separator str API.