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

CString/ToCStr shouldn't have to fail #16485

Closed
SiegeLord opened this issue Aug 13, 2014 · 5 comments
Closed

CString/ToCStr shouldn't have to fail #16485

SiegeLord opened this issue Aug 13, 2014 · 5 comments

Comments

@SiegeLord
Copy link
Contributor

These functions are just so simple that they could easily be adapted not to fail. These API entires are not very beautiful right now, so the additional syntactic overhead of dealing with the result shouldn't be a big deal.

Possible API:

fn new(buf: *const i8, owns_buffer: bool) -> Result<CString, ()>
fn to_c_str(&self) -> Result<CString, ()>
fn with_c_str<T>(&self, f: |*const i8| -> T) -> Result<T, ()>
@aturon
Copy link
Member

aturon commented Aug 13, 2014

cc me

@abonander
Copy link
Contributor

If the Err side of Result is just (), why not use Option?

@SiegeLord
Copy link
Contributor Author

Result can generate an unused result warning, which is especially important to with_c_str above which simply won't call the closure in case of failure. In other cases, it just represents the obvious error... The exact error type isn't really an issue here.

@mzabaluev
Copy link
Contributor

Updated: I have learned enough to completely change my stance on the matter. The comment below is left for historical record.

I beg to differ. It is overwhelmingly more common to not have inner NULs in strings, so such cases can be considered exceptional and not worth the burden of dealing with mandatory Result unwrapping. At least the failing methods should be left around for convenience.

@aturon
Copy link
Member

aturon commented Feb 16, 2015

Closing in favor of the RFC.

@aturon aturon closed this as completed Feb 16, 2015
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this issue Feb 11, 2024
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

No branches or pull requests

5 participants