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

Add ffi::OsString and OsStr #21488

Merged
merged 1 commit into from
Jan 24, 2015
Merged

Add ffi::OsString and OsStr #21488

merged 1 commit into from
Jan 24, 2015

Conversation

aturon
Copy link
Member

@aturon aturon commented Jan 21, 2015

Per RFC 517, this commit introduces platform-native strings. The API is essentially as described in the RFC.

The WTF-8 implementation is adapted from @SimonSapin's implementation. To make this work, some encodign and decoding functionality in libcore is now exported in a "raw" fashion reusable for WTF-8. These exports are not reexported in std, nor are they stable.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@aturon
Copy link
Member Author

aturon commented Jan 21, 2015

Note, this is work in progress and should not be merged until the RFC is accepted.

cc @SimonSapin

/// Yield a `&str` slice if the `OsStr` is valid unicode.
///
/// This conversion may entail doing a check for UTF-8 validity.
pub fn to_str(&self) -> Option<&str> {
Copy link
Member

Choose a reason for hiding this comment

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

to_str => as_str?

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, this can do work (UTF-8 validation).

Copy link
Contributor

Choose a reason for hiding this comment

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

What use is this function? Either you want a string to display, or you don't care about a string. What does it help if you get an Option<&str> except for errornous code that assumes that all pathes are representable as UTF-8?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's akin to http://static.rust-lang.org/doc/master/std/str/fn.from_utf8.html -- basically, there may be cases where you do indeed expect the string to be UTF-8 but you don't want to panic if it's not, you want to handle the error more gracefully. By providing an Option here we combine the checking and the coercion, a very common pattern in std.

@aturon aturon mentioned this pull request Jan 23, 2015
38 tasks
@aturon aturon changed the title [WIP] Add ffi::OsString and OsStr Add ffi::OsString and OsStr Jan 23, 2015
@aturon
Copy link
Member Author

aturon commented Jan 23, 2015

@alexcrichton Pushed an update addressing most of the issues. I didn't add Add just yet; as we discussed today we should probably grow that functionality slowly over time.

Also, as I think I mentioned on IRC, the IntoOsStr approach doesn't really work out for io APIs; I expect AsOsStr to be the main pattern.

use sys_common::{AsInner, IntoInner, FromInner};

/// Owned, mutable OS strings.
#[derive(Show, Clone)]
Copy link
Member

Choose a reason for hiding this comment

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

Could Show be manually implemented instead of derived? (it should delegate to the slice)

@alexcrichton
Copy link
Member

Just a few nits here and there, but otherwise r=me, can't wait for this to start getting implemented!

@aturon
Copy link
Member Author

aturon commented Jan 23, 2015

@bors r=alexcrichton e659d55

@bors
Copy link
Contributor

bors commented Jan 23, 2015

⌛ Testing commit e659d55 with merge 77ecfa3...

@bors
Copy link
Contributor

bors commented Jan 23, 2015

💔 Test failed - auto-win-64-opt

@aturon
Copy link
Member Author

aturon commented Jan 23, 2015

@bors: r=alexcrichton e7850b5

@bors
Copy link
Contributor

bors commented Jan 23, 2015

⌛ Testing commit e7850b5 with merge 9d4358a...

@bors
Copy link
Contributor

bors commented Jan 23, 2015

💔 Test failed - auto-win-64-nopt-t

@aturon
Copy link
Member Author

aturon commented Jan 24, 2015

@bors: r=alexcrichton dc6cd4c

@bors
Copy link
Contributor

bors commented Jan 24, 2015

⌛ Testing commit dc6cd4c with merge f14b6ab...

@bors
Copy link
Contributor

bors commented Jan 24, 2015

💔 Test failed - auto-mac-64-opt

Per [RFC 517](rust-lang/rfcs#575), this commit
introduces platform-native strings. The API is essentially as described
in the RFC.

The WTF-8 implementation is adapted from @SimonSapin's
[implementation](https://github.com/SimonSapin/rust-wtf8). To make this
work, some encodign and decoding functionality in `libcore` is now
exported in a "raw" fashion reusable for WTF-8. These exports are *not*
reexported in `std`, nor are they stable.
@aturon
Copy link
Member Author

aturon commented Jan 24, 2015

@bors: r=alexcrichton c5369eb

bors added a commit that referenced this pull request Jan 24, 2015
Per [RFC 517](rust-lang/rfcs#575), this commit introduces platform-native strings. The API is essentially as described in the RFC.

The WTF-8 implementation is adapted from @SimonSapin's [implementation](https://github.com/SimonSapin/rust-wtf8). To make this work, some encodign and decoding functionality in `libcore` is now exported in a "raw" fashion reusable for WTF-8. These exports are *not* reexported in `std`, nor are they stable.
@bors
Copy link
Contributor

bors commented Jan 24, 2015

⌛ Testing commit c5369eb with merge bb7cc4e...

@bors bors merged commit c5369eb into rust-lang:master Jan 24, 2015
SimonSapin added a commit to SimonSapin/rust-wtf8 that referenced this pull request Jan 28, 2015
@aturon aturon mentioned this pull request Feb 18, 2015
91 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants