Improved docs for CStr, CString, OsStr, OsString #44855

federicomenaquintero · Sep 26, 2017

This expands the documentation for those structs and their corresponding traits, per #29354

rust-highfive · Sep 26, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

cuviper · Sep 26, 2017

CStr and CString are not necessarily UTF-8 at all! If they were, then CStr::to_str() and CString::into_string() would be infallible conversion, not needing a Result.

federicomenaquintero · Sep 26, 2017

Oops, long lines... will fix.

I'll also clarify that CStr/CString are bags of zero-terminated bytes, and UTF-8 only happens when making a string out of them.

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/c_str.rs

-/// This type serves the primary purpose of being able to safely generate a
-/// C-compatible string from a Rust byte slice or vector. An instance of this
+/// This type serves the purpose of being able to safely generate a
+/// C-compatible UTF-8 string from a Rust byte slice or vector. An instance of this


This isn't guaranteed by CStr.

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

@@ -8,7 +8,145 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

-//! Utilities related to FFI bindings.
+//! This module provides utilities to handle C-like strings.  It is


I think that this is a bit misleading because OsString isn't a C string on Windows.

A better way to describe it might be "to handle data across non-Rust interfaces, like other programming languages and the underlying operating system"

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//! borrowed slices of strings with the [`str`] primitive.  Both are
+//! always in UTF-8 encoding, and may contain nul bytes in the middle,
+//! i.e. if you look at the bytes that make up the string, there may
+//! be a `0` among them.  Both `String` and `str` know their length;


nit: the '0' here makes it look like you're referring to a zero digit, not a literal zero. Perhaps use '\0' instead?

another nit: I'd word "know their length" as "store their length explicitly" because technically we "know" the length of a C-string, but it's not computed in O(1) time.

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//!
+//! C strings are different from Rust strings:
+//!
+//! * **Encodings** - C strings may have different encodings.  If


I think that "encoding" here is a bit inaccessible to people who are unfamiliar with how string encoding works. I'd say introduce it with "Rust strings are UTF-8, but C strings may use other encodings. If you're using a string from C, you may have to check its encoding explicitly, rather than just assuming that it's UTF-8 like you can in Rust."

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//! you are bringing in strings from C APIs, you should check what
+//! encoding you are getting.  Rust strings are always UTF-8.
+//!
+//! * **Character width** - C strings may use "normal" or "wide"


"Width" here may be what C uses, but it's again misleading because Unicode has its own specific definition of width. I'd say "size" instead. Instead of using "normal" and "wide," I'd just say directly that C uses two types, char (clarifying that this is different from Rust's type) and wchar_t, which are different sizes.

You can also clarify that wchar_t is referred to by "wide character" but that this doesn't actually reflect the Unicode width, but the size of the character in bytes.

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//! '[Unicode code point]'.
+//!
+//! * **Nul terminators and implicit string lengths** - Often, C
+//! strings are nul-terminated, i.e. they have a `0` character at the


Again, I'd use '\0' here instead of '0'.

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//!
+//! * **Nul terminators and implicit string lengths** - Often, C
+//! strings are nul-terminated, i.e. they have a `0` character at the
+//! end.  The length of a string buffer is not known *a priori*;


No need to use Latin; just say that it isn't stored, but has to be calculated. IMHO we should keep language simple if possible to be more accessible to non-native speakers.

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//! `wcslen()` for `wchar_t`-based ones.  Those functions return the
+//! number of characters in the string excluding the nul terminator,
+//! so the buffer length is really `len+1` characters.  Rust strings
+//! don't have a nul terminator, and they always know their length.


I'd also note in here somewhere that Rust's way of doing it means that you can easily access a string's length, whereas there's an implicit cost to it in C. This also may carry over to CStr if its implementation changes.

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//! so the buffer length is really `len+1` characters.  Rust strings
+//! don't have a nul terminator, and they always know their length.
+//!
+//! * **No nul characters in the middle of the string** - When C


I'd word this as "Internal NULs" as a more succinct version

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//! strings have a nul terminator character, this usually means that
+//! they cannot have nul characters in the middle — a nul character
+//! would essentially truncate the string.  Rust strings *can* have
+//! nul characters in the middle, since they don't use nul


Rather than "don't use nul terminators," it's clearer to say "because NUL doesn't have to mark the end of the string in Rust"

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//! # Representations of non-Rust strings
+//!
+//! [`CString`] and [`CStr`] are useful when you need to transfer
+//! UTF-8 strings to and from C, respectively:


I'd expand this to languages with a C ABI like Python, etc. People should know that a CStr might be necessary when interacting with other languages too.

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//! UTF-8 strings to and from C, respectively:
+//!
+//! * **From Rust to C:** [`CString`] represents an owned, C-friendly
+//! UTF-8 string:  it is valid UTF-8, it is nul-terminated, and has no


Not always valid UTF-8.

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//!
+//! * **From C to Rust:** [`CStr`] represents a borrowed C string; it
+//! is what you would use to wrap a raw `*const u8` that you got from
+//! a C function.  A `CStr` is just guaranteed to be a nul-terminated


"just" seems out of place here; I'd remove it.

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//! * **From C to Rust:** [`CStr`] represents a borrowed C string; it
+//! is what you would use to wrap a raw `*const u8` that you got from
+//! a C function.  A `CStr` is just guaranteed to be a nul-terminated
+//! array of bytes; the UTF-8 validation step only happens when you


"the UTF-8 validation step" is only just mentioned here so I'd just make a separate sentence describing how that works instead, along the lines of "once you have a CStr, you can convert it to a Rust str if it's valid UTF-8, or lossily convert it by adding replacement characters"

clarfon · Sep 27, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon · Sep 27, 2017

src/libstd/ffi/mod.rs

+//! request to convert it to a `&str`.
+//!
+//! [`OsString`] and [`OsStr`] are useful when you need to transfer
+//! strings to and from operating system calls.  If you need Rust


A lot of programmers may not know what system calls are; I'd probably word this as "the operating system itself."

It may also make sense to include examples where this happens, like in opening files and running external commands.

I feel like the "If you need Rust strings out of them [...]" section is kind of redundant and wordy. I'd probably just say that conversions between OsStr and str work very similarly to CStr and leave it at that.

clarfon · Sep 27, 2017

Great work! I've interacted a lot with CStr and OsStr so I added some comments on ways that I think the docs could be made clearer. Hopefully it's more helpful than overwhelming ><

federicomenaquintero · Oct 2, 2017

I've integrated the changes per your comments. How's it look now? :)

clarfon · Oct 2, 2017

Looks good to me! Again, great work! :)

federicomenaquintero · Oct 2, 2017

Thank you!

shepmaster · Oct 6, 2017

Poke @aturon — this is now ready for your masterful reviewing skills!

carols10cents · Oct 9, 2017

Actually, @aturon wasn't available last week and is on PTO this week, so let's try....

r? @steveklabnik

steveklabnik · Oct 11, 2017

steveklabnik requested changes Oct 11, 2017

View changes

This is fantastic, thank you so much!

I have a few little formatting nits, but after that, let's get this merged!

steveklabnik · Oct 11, 2017

src/libstd/ffi/c_str.rs

@@ -149,8 +209,13 @@ pub struct CStr {
 }

 /// An error returned from [`CString::new`] to indicate that a nul byte was found
-/// in the vector provided.
+/// in the vector provided.  While Rust strings may contain nul bytes in the middle,
+/// C strings can't, as that byte would effectively truncate the string.


Could we change this up a bit? We try to have a summary sentence first, then the rest of it. This one has a long summary, and repeats itself since you added the information below. How about:

/// An error indicating that an interior nul byte was found. /// /// While Rust strings may contain nul bytes in the middle, C strings can't, as that byte would effectively /// truncate the string. /// /// This `struct`....

with the correct wrapping, I just guessed here. What do you think?

steveklabnik · Oct 11, 2017

src/libstd/ffi/c_str.rs

+/// that a nul byte was found too early in the slice provided, or one
+/// wasn't found at all for the nul terminator.  The slice used to
+/// create a `CStr` must have one and only one nul byte at the end of
+/// the slice.


Same thing here; don't repeat where it came from, make sure to have a short summary, some space, and then a longer description.

steveklabnik · Oct 11, 2017

src/libstd/ffi/c_str.rs

+/// UTF-8 error was encountered during the conversion.  `CString` is
+/// just a wrapper over a buffer of bytes with a nul terminator;
+/// [`into_string`][`CString::into_string`] performs UTF-8 validation
+/// and may return this error.


steveklabnik · Oct 11, 2017

src/libstd/ffi/c_str.rs

+    /// underlying bytes to construct a new string, ensuring that
+    /// there is a trailing 0 byte.  This trailing 0 byte will be
+    /// appended by this method; the provided data should *not*
+    /// contain any 0 bytes in it.


this isn't a method; could you say "function" instead?

steveklabnik · Oct 11, 2017

src/libstd/ffi/mod.rs

@@ -8,7 +8,156 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

-//! Utilities related to FFI bindings.
+//! This module provides utilities to handle data across non-Rust


I'd keep this short summary, but with a newline between it, so you get the summary. That is:

///! Utilities related to FFI bindings. //! //! This module provides utilities....

steveklabnik · Oct 11, 2017

src/libstd/ffi/mod.rs

+//! C strings are different from Rust strings:
+//!
+//! * **Encodings** - Rust strings are UTF-8, but C strings may use
+//! other encodings.  If you are using a string from C, you should


one space after a period, not two please!

steveklabnik · Oct 11, 2017

src/libstd/ffi/mod.rs

+//! characters; please **note** that C's `char` is different from Rust's.
+//! The C standard leaves the actual sizes of those types open to
+//! interpretation, but defines different APIs for strings made up of
+//! each character type.  Rust strings are always UTF-8, so different


and here, and everywhere 😄

steveklabnik · Oct 12, 2017

steveklabnik approved these changes Oct 12, 2017

View changes

steveklabnik · Oct 12, 2017

Thanks! @bors: r+ rollup

bors · Oct 12, 2017

📌 Commit 5fb8e3d has been approved by steveklabnik

rust-highfive assigned aturon Sep 26, 2017

os_str: Fix too-long lines

4143422

arielb1 added the S-waiting-on-author label Sep 26, 2017

federicomenaquintero added some commits Oct 2, 2017

Remove the implication that CString contains UTF-8 data.

9854e83

Clarify the ffi module's toplevel docs, per @clarcharr's comments

Loading status checks…

50505aa

Fix broken links in documentation

Loading status checks…

d989cd0

shepmaster added S-waiting-on-review and removed S-waiting-on-author labels Oct 6, 2017

rust-highfive assigned steveklabnik and unassigned aturon Oct 9, 2017

kennytm referenced this pull request Oct 13, 2017
Merged
Rollup of 14 pull requests #45261

bors merged commit 5fb8e3d into rust-lang:master Oct 13, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

GuillaumeGomez added a commit to GuillaumeGomez/this-week-in-rust-docs that referenced this pull request Oct 15, 2017

Merge pull request #118 from kennytm/patch-1
rust-lang/rust#44855 should be attributed to @federicomenaquintero

Loading status checks…

fe83f74

rust-lang/rust

Join GitHub today

Improved docs for CStr, CString, OsStr, OsString #44855

Conversation

federicomenaquintero commented Sep 26, 2017

federicomenaquintero added some commits Sep 22, 2017

rust-highfive assigned aturon Sep 26, 2017

This comment has been minimized.

rust-highfive commented Sep 26, 2017

This comment has been minimized.

cuviper commented Sep 26, 2017

This comment has been minimized.

federicomenaquintero commented Sep 26, 2017

arielb1 added the S-waiting-on-author label Sep 26, 2017

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon reviewed Sep 27, 2017 View changes

This comment has been minimized.

clarfon Sep 27, 2017 • edited

This comment has been minimized.

This comment has been minimized.

clarfon commented Sep 27, 2017

federicomenaquintero added some commits Oct 2, 2017

This comment has been minimized.

federicomenaquintero commented Oct 2, 2017

This comment has been minimized.

clarfon commented Oct 2, 2017

This comment has been minimized.

federicomenaquintero commented Oct 2, 2017

shepmaster added S-waiting-on-review and removed S-waiting-on-author labels Oct 6, 2017

This comment has been minimized.

shepmaster commented Oct 6, 2017

This comment has been minimized.

carols10cents commented Oct 9, 2017

rust-highfive assigned steveklabnik and unassigned aturon Oct 9, 2017

steveklabnik requested changes Oct 11, 2017 View changes

steveklabnik left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

federicomenaquintero added some commits Oct 11, 2017

steveklabnik approved these changes Oct 12, 2017 View changes

This comment has been minimized.

steveklabnik commented Oct 12, 2017

This comment has been minimized.

bors commented Oct 12, 2017

kennytm added a commit to kennytm/rust that referenced this pull request Oct 13, 2017

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon reviewed Sep 27, 2017

View changes

clarfon Sep 27, 2017 •

edited

steveklabnik requested changes Oct 11, 2017

View changes

steveklabnik approved these changes Oct 12, 2017

View changes

bors merged commit `5fb8e3d` into rust-lang:master Oct 13, 2017
1 check passed