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.

Sign up for GitHub

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

Jump to bottom

RFC: improve CString construction methods #912

Closed
wants to merge 2 commits into from
Closed

RFC: improve CString construction methods #912

wants to merge 2 commits into from

Conversation

mzabaluev
Copy link
Contributor

Amend the methods available to construct a CString to improve composability and follow the conventions emerging elsewhere in the standard library.

Rendered

@mzabaluev mzabaluev force-pushed the c-string-construction branch from c400cd2 to 984a481 Compare February 26, 2015 01:14
@mzabaluev
Copy link
Contributor Author

Cc @alexcrichton @aturon

@mahkoh
Copy link
Contributor

mahkoh commented Feb 26, 2015

Where are the benchmarks?

@mzabaluev
Copy link
Contributor Author

@mahkoh It's not about performance for now, though the changes widen possibilities for optimization in the future. CString::from_iter is to replace the copy-from-slice implementations of IntoBytes, and its performance should be similarly worse off as Vec::from_iter compared to slice.to_vec() (noticeable, but not dramatic; in theory the optimizer should be able to equalize them). If an IntoCString trait is introduced, which is proposed as an open question, that could inherit the performance benefits of IntoBytes implementations.

@mahkoh
Copy link
Contributor

mahkoh commented Feb 26, 2015

It's not about performance for now

You might not care about performance but I do.

though the changes widen possibilities for optimization in the future.

The opposite is true. The optimizations possible with a &[u8] are a strict superset of the optimizations possible with an u8 Iterator.

@mzabaluev
Copy link
Contributor Author

The optimizations possible with a &[u8] are a strict superset of the optimizations possible with an u8 Iterator.

Given that slice::Iter reports accurate length and its implementation of next can in theory be turned into a loop index by the optimizer with invariants optimized away, I don't see principal difference for code that iterates forward and uses size_hint to optimize. In practice, benchmarks I've ran while working on rust-lang/rust#22681 do show a difference, but it needs a hard look into the generated code to see where it is coming from.

@mahkoh
Copy link
Contributor

mahkoh commented Feb 26, 2015

In practice, benchmarks I've ran while working on #22681 do show a difference

The current CString::new performance can easily be improved by an order of magnitude.

but it needs a hard look into the generated code to see where it is coming from.

Go for it.

@mzabaluev
Copy link
Contributor Author

The current CString::new performance can easily be improved by an order of magnitude.

Then the same can be done for an implementation of IntoCString for slices, if micro-optimizations is the current concern at all for 1.0. I got the vibe that discarding redundant and poorly composable traits and methods has higher priority for now.

@nikomatsakis
Copy link
Contributor

@mahkoh I have no dog in this race (yet), but I find the tone of your comments here on this thread to be unnecessarily antagonistic. It is good and reasonable to inquire into the effect that this change has on performance, but we're all on the same team here.

alexcrichton
alexcrichton reviewed Feb 27, 2015
View reviewed changes
text/0000-c-string-construction.md

1. The `IntoBytes` trait does not seem wholly justified: it falls short of
supporting `IntoIterator`, and has become yet another special-interest trait
to care about when designing APIs producing string-like data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this trait was introduced but largely only as a temporary measure. With generic conversion traits this would be much less ad-hoc. The semi-current-plan is to leave IntoBytes as #[unstable] until we have conversion traits at which point we can switch to using those.

@alexcrichton alexcrichton mentioned this pull request Mar 2, 2015
Merged
@alexcrichton alexcrichton self-assigned this Mar 5, 2015
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 5, 2015
@alexcrichton
std: Stabilize the ffi module
628f5d2 
The two main sub-modules, `c_str` and `os_str`, have now had some time to bake
in the standard library. This commits performs a sweep over the modules adding
various stability tags.

The following APIs are now marked `#[stable]`

* `OsString`
* `OsStr`
* `OsString::from_string`
* `OsString::from_str`
* `OsString::new`
* `OsString::into_string`
* `OsString::push` (renamed from `push_os_str`, added an `AsOsStr` bound)
* various trait implementations for `OsString`
* `OsStr::from_str`
* `OsStr::to_str`
* `OsStr::to_string_lossy`
* `OsStr::to_os_string`
* various trait implementations for `OsStr`
* `CString`
* `CStr`
* `NulError`
* `CString::new` - this API's implementation may change as a result of
  rust-lang/rfcs#912 but the usage of `CString::new(thing)` looks like it is
  unlikely to change. Additionally, the `IntoBytes` bound is also likely to
  change but the set of implementors for the trait will not change (despite the
  trait perhaps being renamed).
* `CString::from_vec_unchecked`
* `CString::as_bytes`
* `CString::as_bytes_with_nul`
* `NulError::nul_position`
* `NulError::into_vec`
* `CStr::from_ptr`
* `CStr::as_ptr`
* `CStr::to_bytes`
* `CStr::to_bytes_with_nul`
* various trait implementations for `CStr`

The following APIs remain `#[unstable]`

* `OsStr*Ext` traits remain unstable as the organization of `os::platform` is
  uncertain still and the traits may change location.
* `AsOsStr` remains unstable as generic conversion traits are likely to be
  rethought soon.

The following APIs were deprecated

* `OsString::push_os_str` is now called `push` and takes `T: AsOsStr` instead (a
  superset of the previous functionality).
@mzabaluev
Rewrite as a second-string proposal to generic conversion traits
28215b6 
IntoCString is promoted to proposed changes after meeting no objections
as an ergonomic replacement for IntoBytes.
@aturon
Copy link
Member

aturon commented Apr 3, 2015

The generic conversion traits did land (and stabilize), meaning that many of the goals here have been resolved. The current constructors still work with Vec<u8>, but this seems an acceptable risk (and there are various ways we could change the internal representation and construction later.) So I'm going to close out this RFC.

Thanks @mzabaluev!

@aturon aturon closed this Apr 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mzabaluev @mahkoh @nikomatsakis @aturon @alexcrichton