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

RFC: get panics out of std::ffi::CString #840

Merged
merged 3 commits into from Feb 18, 2015

Conversation

Projects
None yet
6 participants
@mzabaluev
Copy link
Contributor

mzabaluev commented Feb 14, 2015

Bring CString in line with other library functions on the failure mode conventions.
There should be no panic lurking.

Rendered

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Feb 14, 2015

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Feb 14, 2015

Alternative: Add a checked_from_slice() method, just like we have x/y which may panic and x.checked_div(y) which does not.

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Feb 15, 2015

@kennytm I have added a paragraph on why simply adding functions is worse than changing the "shortest name" API.

In case of division, a zero divisor arguably is a programmer error. A restriction on NULs in a string, on the other hand, is arbitrary for the value domain in general (a Unicode text with NUL characters is valid). There are also performance concerns regarding the implementation of division.

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Feb 15, 2015

That said, a migration path is probably needed whereby the Result-returning functions are added under temporary names and the current functions deprecated. Suggestions?

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Feb 17, 2015

Thanks so much for throwing this together! I'm increasingly in favor of this approach for C strings.

cc @wycats @nikomatsakis @brson @huonw

@pcwalton I know CString is somewhat common in Servo, do you all have a preference here?

@huonw

This comment has been minimized.

Copy link
Member

huonw commented Feb 17, 2015

Seems fine to me.

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Feb 17, 2015

@aturon +1 from me.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Feb 18, 2015

After discussion here and on discuss, we've decided to move forward with this long-requested change. Thanks again, @mzabaluev! We'll try to land this ahead of alpha2 if possible.

Tracking issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.