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

API Docs: ffi #29354

Closed
8 tasks
steveklabnik opened this issue Oct 26, 2015 · 23 comments
Closed
8 tasks

API Docs: ffi #29354

steveklabnik opened this issue Oct 26, 2015 · 23 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-medium Medium priority

Comments

@steveklabnik
Copy link
Member

steveklabnik commented Oct 26, 2015

Part of #29329

http://doc.rust-lang.org/std/ffi/

Here's what needs to be done to close out this issue:

  • the module docs are almost non-existent, and should talk about how this isn't so much FFI bindings as it is stuff useful for interop with C, which is FFI of course, but that's it. It's not a comprehensive FFI solution.
  • CStr should make the comparison with str. it needs a lot of links.
  • CString should make the comparison with String. It also needs a lot of links.
  • FromBytesWithNulError should use the iterator style boilerplate for linking to where it came from.
  • IntoStringError same
  • NulError same.
  • OsStr should make the str comparison, and needs a lot more elaboartion. It should look like CStr.
  • OsString should make the comparison with String, and should look a lot like CString.
@seankerr
Copy link

One of the constant reminders I tell myself when working with FFI is that CStr consumes a C NULL terminated string, and that CString represents one. It would be nice if the documentation for both of these structs indicated their purpose, and contained basic FFI examples for using them.

@jethrogb
Copy link
Contributor

OsString docs say

allowing a Rust string to be converted into an "OS" string with no cost.

I'm not sure if that's entirely correct and it's slightly confusing. On Windows at least you need another step through OsStrExt that is costly. Also, the reverse conversion is always costly (O(n)) to do safely.

@steveklabnik steveklabnik added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Mar 8, 2017
@steveklabnik
Copy link
Member Author

I am happy to mentor anyone who wants to tackle this issue.

@seankerr
Copy link

seankerr commented Mar 9, 2017

@steveklabnik I'd be happy to write the docs on these provided some guidance. When are you available for a quick doc lesson?

@steveklabnik
Copy link
Member Author

@seankerr awesome! We can do it asynchronously on this issue, or synchronously if you ping me on IRC, I'm around most often during 10-6ish EST, but at other times too, and sometimes I'm in meetings.

@steveklabnik steveklabnik added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Mar 10, 2017
@steveklabnik
Copy link
Member Author

@seankerr I've updated the issue with some more details here

@steveklabnik steveklabnik added P-medium Medium priority E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. and removed A-docs labels Mar 24, 2017
@jck
Copy link

jck commented Mar 25, 2017

I would also like to volunteer for writing the ffi docs.

@steveklabnik
Copy link
Member Author

Great, thank you @jck ! Let me know if you need any help.

@DanielKeep
Copy link
Contributor

Contrary to what some might assume, CStr and CString do not convert to or from the encoding C uses for zero-terminated multibyte (*char) strings. With the exception of strings that are explicitly UTF-8, users should not attempt to use CStr or CString to convert between C strings and Rust strings.

Potential rule of thumb: if it's meant to go into or come out of libc, it's probably not compatible with the CStr / CString methods that take/produce Rust strings.

As this is a relatively reasonable assumption users might make (particularly prompted by the significant body of existing external documentation about interop between Rust and C that points users to these types), the docs should probably make this point very clear.

(For reference: an internals thread about CStr and CString.)

@kornelski
Copy link
Contributor

kornelski commented Mar 29, 2017

In the discussion @DanielKeep mentions there was confusion about whether "C-compatible string" means just char* as an 8-bit NUL-terminated string, or all string types that C may use, including wchar_t APIs that are popular on Windows.

So it may be worth to clarify in the docs that CString is for the one particular 8-bit string type, and will not work with other types of strings that are in some C APIs and dialects.

@kornelski
Copy link
Contributor

kornelski commented Mar 29, 2017

Currently docs don't explicitly say whether 8-bit encodings other than UTF-8 are allowed in CString (and whether the other encodings have to be ASCII-based. The current Debug implementation is not correct for EBCDIC).

There are methods for converting from and to UTF-8-encoded strings, but does it meant that CString bytes may be in UTF-8 or must be in UTF-8? Can I create CString from ISO-8859-1 encoded bytes if I never call to_str() on it?

@jethrogb
Copy link
Contributor

I don't think it's useful to have this discussion in two places. The current CString API is clearly meant for null-terminated byte strings with unspecified encoding, as all the Vec<u8>-based functions are infallible. The fallible str/String conversion functions are purely there for convenience if you know it's UTF-8.

@frewsxcv
Copy link
Member

Might be good to mention at the module level that FFI stands for "foreign function interface".

@QuietMisdreavus
Copy link
Member

Small example of something to include in OsString: into_string doesn't just check for UTF-8; it defers to a platform-specific conversion, e.g. WTF-8 on Windows.

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jul 24, 2017
@federicomenaquintero
Copy link
Contributor

I'm familiar with this module and would be happy to update the docs. Should I just start submitting PRs?

@QuietMisdreavus
Copy link
Member

@federicomenaquintero Absolutely! Start working on the bits you're willing to take on, and submit PRs whenever you're ready. You don't have to tackle the whole list up top at once.

@federicomenaquintero
Copy link
Contributor

I'll continue with these docs next week. Do those two commits look like going in the right direction?

@federicomenaquintero
Copy link
Contributor

I'm a bit miffed by two notes in the documentation.

  • CStr::from_ptr says, "Note: This operation is intended to be a 0-cost cast but it is currently implemented with an up-front calculation of the length of the string. This is not guaranteed to always be the case."

  • Correspondingly, CStr::to_bytes says, "Note: This method is currently implemented as a 0-cost cast, but it is planned to alter its definition in the future to perform the length calculation whenever this method is called."

It sounds like at some point CStr was intended to really only wrap a *const u8, but it was then changed to validate the string at construction and store a slice instead. I don't see a problem with this; it keeps garbage from entering the Rust-space :)

I'm happy to note in the docs where validation is being done in linear time. However, should we keep those specific comments around? Or are there plans to change CStr to really being a pointer wrapper with lazy validation? I didn't find an issue about this.

@steveklabnik
Copy link
Member Author

That's a good question. @rust-lang/libs?

@alexcrichton
Copy link
Member

If CStr contained just *const u8 then you'd be able to do mem::size_of::<CStr>() or something like struct Foo { a: CStr }, where we actually were hoping to prevent that by forcing you to always have a lifetime behind a reference like &'a CStr or struct Foo<'a> { a: &'a CStr }. The only way we can force you to have such a reference though is to make CStr a dynamically sized type, namely struct CStr([u8]).

Now what we want to have is something like:

struct CStr(u8);

impl !Sized for CStr {}

mem::size_of::<CStr>(); // compile error
mem::size_of::<&CStr>() == mem::size_of::<usize>();

That's probably unlikely to happen for awhile though :(

kennytm added a commit to kennytm/rust that referenced this issue Oct 13, 2017
…eklabnik

Improved docs for CStr, CString, OsStr, OsString

This expands the documentation for those structs and their corresponding traits, per rust-lang#29354
@federicomenaquintero
Copy link
Contributor

This is merged; can the issue be closed? (Also, in the parent #29329)

@steveklabnik
Copy link
Member Author

👍 ❤️

@federicomenaquintero
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-medium Medium priority
Projects
None yet
Development

No branches or pull requests