Implement [OsStr]::join #96744

est31 · 2022-05-05T20:01:33Z

Implements join for OsStr and OsString slices:

    let strings = [OsStr::new("hello"), OsStr::new("dear"), OsStr::new("world")];
    assert_eq!("hello dear world", strings.join(OsStr::new(" ")));

This saves one from converting to strings and back, or from implementing it manually.

rust-highfive · 2022-05-05T20:01:36Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-05-05T20:01:37Z

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

est31 · 2022-05-05T20:01:59Z

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

thomcc · 2022-05-06T01:27:46Z

@bors r+

bors · 2022-05-06T01:27:48Z

📌 Commit 4fcbc53 has been approved by thomcc

…piler-errors Rollup of 7 pull requests Successful merges: - rust-lang#96174 (mark ptr-int-transmute test as no_run) - rust-lang#96639 (Fix typo in `offset_from` documentation) - rust-lang#96704 (Add rotation animation on settings button when loading) - rust-lang#96730 (Add a regression test for rust-lang#64173 and rust-lang#66152) - rust-lang#96741 (Improve settings loading strategy) - rust-lang#96744 (Implement [OsStr]::join) - rust-lang#96747 (Add `track_caller` to `DefId::expect_local()`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

faptc · 2022-05-06T12:02:10Z

library/std/src/ffi/os_str.rs

+
+    fn join(slice: &Self, sep: &OsStr) -> OsString {
+        let Some(first) = slice.first() else {
+            return OsString::new();


Why not use slice::split_first?

Didn't know the function existed, thanks! Feel free to file a PR.

Ideally, one would make an optimized implementation like there is for str slices, but OsStr is platform dependent in its representation as well as the invariants, so it's not as easy as just exporting that function from alloc, probably as perma unstable and #[doc(hidden)] and then using it, at least not on unixes. It's a larger project and I wasn't sure if the PR was going to accepted at all, so I went with the simplest implementation I could think of, establishing a baseline that can later be improved. Maybe a good next step would be to use the join_generic_copy function on Unix and fall back on the current implementation on other operating systems, but without benchmarking I don't know how much that is an improvement.

m-ou-se · 2022-05-07T09:26:28Z

This PR changed the public stable API. [OsStr]::join() now works without any unstable feature flags. This should've had an FCP.

m-ou-se · 2022-05-07T09:29:31Z

library/std/src/ffi/os_str.rs

@@ -1222,6 +1222,23 @@ impl OsStr {
    }
 }

+#[unstable(feature = "slice_concat_ext", issue = "27747")]


The tracking issue #27747 was closed a while ago.

I used the same tracking issue the other impl blocks for that trait have. The reason to close the tracking issue was because the feature is to be perma-unstable: #27747 (comment) . I'm not sure about changes in policy, but in the older times tracking issues for perma-unstable things would remain open indefinitely.

est31 · 2022-05-07T22:16:37Z

This PR changed the public stable API. [OsStr]::join() now works without any unstable feature flags. This should've had an FCP.

Willing to start one @m-ou-se ? If it doesn't make it through one can still revert the PR.

thomcc · 2022-05-08T08:55:59Z

Wow, I have no idea why I r+ed this! It wasn’t even assigned to me. I think I had confused it with another PR! 😰

thomcc · 2022-05-08T09:43:01Z

Ugh, this is really concerning actually. I don't remember this PR at all (not my review or even the change itself)... The fact that it was r+ed on the 5th in the afternoon, is... very weird.

That morning, I visited an urgent care clinic that morning (woke with mysterious+intense stomach pain), and... I basically came home, did a little work, then stopped and went to sleep through the whole time when the comment was made. Or... that's what I thought? I guess maybe I did stuff online that I... genuinely don't at all remember? :(

Anyway, I'm sorry, I don't know what happened. But I don't expect this to be particularly reassuring... (it isn't to me).

est31 · 2022-05-08T14:41:31Z

@thomcc it's sad to hear that you were in such a bad state of health. I hope you are doing better now.

You were assigned initially but then later I made the r? command per advice of the bot, as this is indeed a change of the stable public API, which assigned someone else. I'm wondering what the way forward is for this change.

thomcc · 2022-05-08T16:50:50Z

Yeah that's normal. The confusing part is that I don't remember even seeing the PR, let alone reviewing it, and thought I was asleep during the time github says I make the review. I guess I'll change my password and such even though the circumstances definitely make it more likely that I did it (and just... lost that time that I thought I was sleeping 😓).

I've put up a PR reverting it. I think the appropriate thing would be to resubmit and do it properly. In particular, this change is not obviously something we want. For example, I am not sure if fromWTF8(toWTF8(a) + toWTF8(b)) == (a + b) for all a and b.

I'm terribly sorry for the trouble here, and feel awful about causing it.

Revert "Implement [OsStr]::join", which was merged without FCP. This reverts commit 4fcbc53, see rust-lang#96744. (I'm terribly sorry, and truly don't remember r+ing it, or even having seen it before yesterday, which is... genuinely very worrisome for me). r? `@m-ou-se`

est31 · 2022-05-09T20:06:48Z

Second attempt is in: #96881

Implement [OsStr]::join Implements join for `OsStr` and `OsString` slices: ```Rust let strings = [OsStr::new("hello"), OsStr::new("dear"), OsStr::new("world")]; assert_eq!("hello dear world", strings.join(OsStr::new(" "))); ```` This saves one from converting to strings and back, or from implementing it manually. This PR has been re-filed after rust-lang#96744 was first accidentally merged and then reverted. The change is instantly stable and thus: r? rust-lang/libs-api `@rustbot` label +T-libs-api -T-libs cc `@thomcc` `@m-ou-se` `@faptc`

Revert "Implement [OsStr]::join", which was merged without FCP. This reverts commit 4c460de02e132ef05e25888f0254a9eab894a223, see rust-lang/rust#96744. (I'm terribly sorry, and truly don't remember r+ing it, or even having seen it before yesterday, which is... genuinely very worrisome for me). r? `@m-ou-se`

Implement [OsStr]::join

4fcbc53

rust-highfive assigned thomcc May 5, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 5, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 5, 2022

rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 5, 2022

rust-highfive assigned m-ou-se and unassigned thomcc May 5, 2022

rustbot removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 5, 2022

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 May 6, 2022

compiler-errors mentioned this pull request May 6, 2022

Rollup of 7 pull requests #96759

Merged

bors merged commit 8bcf4b0 into rust-lang:master May 6, 2022

rustbot added this to the 1.62.0 milestone May 6, 2022

faptc reviewed May 6, 2022

View reviewed changes

m-ou-se reviewed May 7, 2022

View reviewed changes

thomcc mentioned this pull request May 8, 2022

Revert "Implement [OsStr]::join", which was merged without FCP. #96841

Merged

est31 mentioned this pull request May 9, 2022

Implement [OsStr]::join #96881

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement [OsStr]::join #96744

Implement [OsStr]::join #96744

est31 commented May 5, 2022

rust-highfive commented May 5, 2022

rust-highfive commented May 5, 2022

est31 commented May 5, 2022

thomcc commented May 6, 2022

bors commented May 6, 2022

faptc May 6, 2022

est31 May 6, 2022

m-ou-se commented May 7, 2022

m-ou-se May 7, 2022

est31 May 7, 2022

est31 commented May 7, 2022

thomcc commented May 8, 2022

thomcc commented May 8, 2022 •

edited

Loading

est31 commented May 8, 2022

thomcc commented May 8, 2022 •

edited

Loading

est31 commented May 9, 2022

Implement [OsStr]::join #96744

Implement [OsStr]::join #96744

Conversation

est31 commented May 5, 2022

rust-highfive commented May 5, 2022

rust-highfive commented May 5, 2022

est31 commented May 5, 2022

thomcc commented May 6, 2022

bors commented May 6, 2022

faptc May 6, 2022

Choose a reason for hiding this comment

est31 May 6, 2022

Choose a reason for hiding this comment

m-ou-se commented May 7, 2022

m-ou-se May 7, 2022

Choose a reason for hiding this comment

est31 May 7, 2022

Choose a reason for hiding this comment

est31 commented May 7, 2022

thomcc commented May 8, 2022

thomcc commented May 8, 2022 • edited Loading

est31 commented May 8, 2022

thomcc commented May 8, 2022 • edited Loading

est31 commented May 9, 2022

thomcc commented May 8, 2022 •

edited

Loading

thomcc commented May 8, 2022 •

edited

Loading