Uh oh!
There was an error while loading. Please reload this page.
Owned conversions for CString - #28977
Conversation
rust-highfive
commented
Oct 11, 2015
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pcwalton (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. 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. |
There was a problem hiding this comment.
Unlike OsString's into_string method this actually has some extra contextual information to give back (e.g. the str::Utf8Error). Along those lines this may want to return its own custom error type (e.g. the same as FromUtf8Error but with a CString instead of String.
There was a problem hiding this comment.
Could you suggest what this type should be called? Would the best course of action be to just create std::ffi::FromUtf8Error or name it CStringFromUtf8Error or..?
There was a problem hiding this comment.
This would probably be called IntoStringError
alexcrichton
commented
Oct 12, 2015
Thanks @arcnmx! These look pretty good to me, but gonna tag with |
alexcrichton
commented
Oct 15, 2015
We chatted about this in the libs team meeting today and were fine moving forward with it. We also thought that it would be a good idea to hold off on an |
arcnmx
commented
Oct 17, 2015
Included the requested comment and created an |
There was a problem hiding this comment.
Should this instead be called into_cstring? There's some precedent here (NulError::into_vec and FromUtf8Error::into_bytes) but they're not even consistent so I wasn't sure...
There was a problem hiding this comment.
Yeah let's call this into_cstring for now to stick with into_<type> of the other errors.
There was a problem hiding this comment.
I've created #29157 to connect all these features, to, so could you connect them via issue to that and then also update the feature name here to something like cstring_into or something similar?
arcnmx
commented
Oct 19, 2015
Changes made! |
alexcrichton
commented
Oct 19, 2015
Looks good to me! Could you also squash down to one commit? Other than that r=me |
arcnmx
commented
Oct 19, 2015
Squashed! |
alexcrichton
commented
Oct 19, 2015
`OsString` has these sorts of conversions, while `CString` has been missing them. I'm iffy on `into_string` simply because the return type would be better off as `FromUtf8Error<CString>`, which of course isn't generic 😢 Also should a different/new feature gate be used?
OsStringhas these sorts of conversions, whileCStringhas been missing them. I'm iffy oninto_stringsimply because the return type would be better off asFromUtf8Error<CString>, which of course isn't generic 😢Also should a different/new feature gate be used?