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

Additions/improvements for doc examples. #42749

Merged
merged 12 commits into from
Jun 21, 2017

Conversation

frewsxcv
Copy link
Member

No description provided.

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2017
@carols10cents
Copy link
Member

r? @GuillaumeGomez

/// ```
///
/// Had `c_str` contained invalid unicode, the `to_string_lossy` call might
/// have returned `Cow::Borrowed("fo�")`.
Copy link
Member

Choose a reason for hiding this comment

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

How about adding an example for the invalid Unicode case? Either way is definitely wouldn't return Cow::Borrowed it would return Cow::Owned.

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured it'd be undefined behavior to create a string w/ invalid unicode, as your comment here.

Either way is definitely wouldn't return Cow::Borrowed it would return Cow::Owned.

Yep, good catch, I'll change that

Copy link
Member

Choose a reason for hiding this comment

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

I figured it'd be undefined behavior to create a string w/ invalid unicode

Strings and strs have to contain valid UTF-8 but CStrings and Cstrs are just sequences of any none null bytes. The following example would be fine:

use std::borrow::Cow;
use std::ffi::CStr;

let c_str = CStr::from_bytes_with_nul(b"foo\xc3\0").unwrap();
assert_eq!(c_str.to_string_lossy(), "foo�");

It would also be more correct to say "(in)valid UTF-8" rather than "(in)valid unicode" in this case.

Copy link
Member

Choose a reason for hiding this comment

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

It might be worth borrowing from the example in String::from_utf8_lossy, which uses the input string b"Hello \xF0\x90\x80World" to demonstrate the replacement.

@QuietMisdreavus
Copy link
Member

Looks good, other than the discussion surrounding to_string_lossy. If you can update those examples (on both CStr and OsStr) then I'm okay with everything else.

@frewsxcv
Copy link
Member Author

Looks good, other than the discussion surrounding to_string_lossy. If you can update those examples (on both CStr and OsStr) then I'm okay with everything else.

Fixed the failing doc test and updated the CStr::to_string_lossy examples. I ditched my changes to OsStr. I don't think there's a way to create an OsStr from bytes (in a cross platform manner) like we can for CStr so I don't know how to demonstrate an OsStr containing invalid UTF8.

/// Create a `CString`, pass ownership to an `extern` function (via raw pointer), then retake
/// ownership with `from_raw`:
///
/// ```ignore
Copy link
Member

Choose a reason for hiding this comment

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

Could you change this to ```no_run? Also the some_external_function(raw) on line 307 should be some_extern_function(raw).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll get linker errors upon building, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Latest force push addresses the typo, good catch

Copy link
Member

Choose a reason for hiding this comment

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

@frewsxcv Currently no_run (and compile_fail as well) will stop after analysis. It won't even run codegen.

@frewsxcv frewsxcv force-pushed the frewsxcxv/doc-examples branch 2 times, most recently from 97de688 to 94b09dd Compare June 20, 2017 12:33
@frewsxcv
Copy link
Member Author

Changed ignore to no_run in the latest force push

@bors
Copy link
Contributor

bors commented Jun 20, 2017

☔ The latest upstream changes (presumably #42716) made this pull request unmergeable. Please resolve the merge conflicts.

@frewsxcv
Copy link
Member Author

Rebased, addressed merge conflicts

@QuietMisdreavus
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 20, 2017

📌 Commit 58bbe1d has been approved by QuietMisdreavus

@frewsxcv
Copy link
Member Author

@bors rollup

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 20, 2017
…QuietMisdreavus

Additions/improvements for doc examples.

None
@arielb1 arielb1 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 Jun 20, 2017
bors added a commit that referenced this pull request Jun 20, 2017
Rollup of 6 pull requests

- Successful merges: #42271, #42717, #42728, #42749, #42756, #42772
- Failed merges:
@bors bors merged commit 58bbe1d into rust-lang:master Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants