Conversation
|
r? @dtolnay |
Dylan-DPC-zz
added
the
S-waiting-on-review
mlodato517
left a comment
mlodato517
left a comment
There was a problem hiding this comment.
One small nitpick/question on where to place the doc links in the comments. Otherwise looks good to me but I'm too new to rust to approve or anything :-)
dtolnay
left a comment
dtolnay
left a comment
There was a problem hiding this comment.
This looks great to me.
However, there is no such thing as an unstable impl of a stable trait for a stable type, so the new OsString trait impls will require consensus from the libs team to merge. Could you move just the IndexMut and DerefMut impls to a separate PR? We'll do an FCP to stabilize those, which should be uncontroversial, and then rebase and land the unstable ascii-related methods.
Thanks!
|
@TyPR124 👋 from triage, any progress? |
|
Still waiting on #70048 to be merged. Not sure how long that takes but it is in final comment period now. |
Dylan-DPC-zz
added
S-blocked
Allow obtaining &mut OsStr
```rust
impl DerefMut for OsString {...} // type Target = OsStr
impl IndexMut<RangeFull> for OsString {...} // type Output = OsStr
```
---
This change is pulled out of rust-lang#69937 per @dtolnay
This implements `DerefMut for OsString` to allow obtaining a `&mut OsStr`. This also implements `IndexMut for OsString`, which is used by `DerefMut`. This pattern is the same as is used by `Deref`.
This is necessary to for methods like `make_ascii_lowercase` which need to mutate the underlying value.
|
☔ The latest upstream changes (presumably #70451) made this pull request unmergeable. Please resolve the merge conflicts. |
dtolnay
added
S-waiting-on-author
|
Ready to rebase and then I can approve. |
|
I hope I did that right. Do I need to open a tracking issue for this? Not totally sure how that works to be honest. |
|
Thanks, this is just about ready. Yes, please follow the workflow in https://github.com/rust-lang/rust/issues/new/choose to open a tracking issue and then put the issue number in the attributes in the PR. |
|
@bors r+ |
|
📌 Commit 271d43b has been approved by |
bors
added
S-waiting-on-bors
Rollup of 5 pull requests Successful merges: - rust-lang#69937 (ASCII methods on OsStr) - rust-lang#70235 (Validate git setup before accessing functionality) - rust-lang#70503 (rename Scalar::{ptr_null -> null_ptr} and add "machine_" prefix like elsewhere) - rust-lang#70508 (Miri: use more specialized Scalar::from_ constructors where appropriate) - rust-lang#70510 (fix TryEnterCriticalSection return type) Failed merges: r? @ghost
6 participants
Would close #69566
I don't know enough about encodings to know if this is a valid change, however the comment on the issue suggests it could be.
This does two things:
Makes ASCII methods available on OsStr
Makes it possible to obtain a
&mut OsStr. This is necessary to actually useOsStr::make_ascii_*casemethods since they modify the underlying value. As far as I can tell, the only way to modify a&mut OsStris via the methods I just added.My original hope was to have these methods on
OsStrExtfor Windows, since the standard library already assumesmake_ascii_uppercaseis valid in Windows (see the change I made to windows/process.rs). If it is found these are not valid changes on non-Windows platforms, I can move the methods to the ext trait instead.