-
Notifications
You must be signed in to change notification settings - Fork 20
Add a CStr::count_bytes()
method
#256
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Comments
Not yet approved of course, but this is small enough that I got started on work. Tracking issue: rust-lang/rust#114441 Implementation PR: rust-lang/rust#114443 |
That same performance regression will also inevitably happen to |
I agree, we should not hamstring this on purpose. A similar doc note would be appropriate, so we don't make promises we might not want to keep, but we would add this to the performance considerations of making that switch. |
That is all fair. I have updated the proposal to not call |
We discussed this in last week's libs-api meeting. Since the plan is still for The |
in that case there should be a doc alias for |
Thanks @m-ou-se. I updated this proposal and the implementation to use
|
isn't perhaps you were thinking of |
Yes I am, please ignore me 🙂 |
I would be happy to accept a PR for unstable |
Implement `cstr_count_bytes` This has not yet been approved via ACP, but it's simple enough to get started on. - ACP: rust-lang/libs-team#256 - Tracking issue: rust-lang#114441 `@rustbot` label +T-libs-api
Implement `cstr_count_bytes` This has not yet been approved via ACP, but it's simple enough to get started on. - ACP: rust-lang/libs-team#256 - Tracking issue: rust-lang/rust#114441 `@rustbot` label +T-libs-api
Implement `cstr_count_bytes` This has not yet been approved via ACP, but it's simple enough to get started on. - ACP: rust-lang/libs-team#256 - Tracking issue: rust-lang/rust#114441 `@rustbot` label +T-libs-api
Implement `cstr_count_bytes` This has not yet been approved via ACP, but it's simple enough to get started on. - ACP: rust-lang/libs-team#256 - Tracking issue: rust-lang/rust#114441 `@rustbot` label +T-libs-api
Uh oh!
There was an error while loading. Please reload this page.
Proposal
Problem statement
Currently, the best way to evaluate the length of a
CStr
iscs.to_bytes().len()
. This is not ergonomic, and may not be the best option whenCStr
becomes a thin pointer.Motivating examples or use cases
For both a fat and thin
CStr
, this will provide a more intuitive method to get the length. WhenCStr
becomes thin,CStr::count_bytes
will be able to callstrlen
directly without going through the less directto_bytes
>to_bytes_with_nul
>slice::from_raw_parts
>len
.Solution sketch
Add a method
CStr::count_bytes
that returns its length. Currently, we can use aself.inner.len() - 1
to get the length of the string, which is a constant time operation. OnceCStr
becomes thin, this will become anO(1)
call tostrlen
- we just need to make it clear in documentation that this will have a performance regression at some point (as we do for theto_bytes()
methods).The API is simple:
Once thin, this will become:
Alternatives
The status quo: accessing length via
.to_bytes().len()
. This ACP intends to improve upon this option.const
nessCurrenly
CStr::from_ptr
uses a version ofstrlen
that calls libc's implementation normally, or a naive Rust implementation when const. This works viaconst_eval_select
which to my knowledge hasn't been okayed for internal use. So, we will not be able to makeCStr::count_bytes
const stable until either that happens, or we decide our naivestrlen
is always OK (makingCStr::new
const is also blocked by this, or by the switch to a thin pointer).My current implementation gates the constness under
const_cstr_from_ptr
which has the same const-stable blocker.cc @oli-obk because of your comment on
const_eval_select
here rust-lang/rust#107624 (comment) (I believe this is the RFC: rust-lang/rfcs#3352)Naming
The name
count_bytes
is up for some bikeshedding.Links and related work
CStr::len
method when stabilizingCStr::is_empty
: Tracking Issue for CStr::is_empty rust#102444 (comment)CStr
methods based on whether or not we haveconst_eval_select
, here: Stabilizeconst_cstr_methods
rust#107624 (comment)What happens now?
This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.
The text was updated successfully, but these errors were encountered: