Stabilize feature `cstr_from_bytes_until_nul` #107429

tgross35 · 2023-01-28T22:40:21Z

This PR seeks to stabilize cstr_from_bytes_until_nul.

Partially addresses #95027

This function has only been on nightly for about 10 months, but I think it is simple enough that there isn't harm discussing stabilization. It has also had at least a handful of mentions on both the user forum and the discord, so it seems like it's already in use or at least known.

This needs FCP still.

Comment on potential discussion points:

eventual conversion of CStr to be a single thin pointer: this function will still be useful to provide a safe way to create a CStr after this change.
should this return a length too, to address concerns about the CStr change? I don't see it as being particularly useful, and it seems less ergonomic (i.e. returning Result<(&CStr, usize), FromBytesUntilNulError>). I think users that also need this length without the additional strlen call are likely better off using a combination of other methods, but this is up for discussion
CString::from_vec_until_nul: this is also useful, but it doesn't even have a nightly implementation merged yet. I propose feature gating that separately, as opposed to blocking this CStr implementation on that

Possible alternatives:

A user can use from_bytes_with_nul on a slice up to my_slice[..my_slice.iter().find(|c| c == 0).unwrap()]. However; that is significantly less ergonomic, and is a bit more work for the compiler to optimize compared the direct memchr call that this wraps.

New stable API

// both in core::ffi

pub struct FromBytesUntilNulError(());

impl CStr {
    pub const fn from_bytes_until_nul(
        bytes: &[u8]
    ) -> Result<&CStr, FromBytesUntilNulError>
}

cc @ericseppanen original author, @Mark-Simulacrum original reviewer, @m-ou-se brought up some issues on the thin pointer CStr

@rustbot modify labels: +T-libs-api +needs-fcp

rustbot · 2023-01-28T22:40:28Z

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-01-28T22:40:31Z

The Miri subtree was changed

cc @rust-lang/miri

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

cuviper · 2023-01-28T22:46:31Z

r? libs-api

joshtriplett · 2023-01-29T01:54:53Z

@rfcbot merge

rfcbot · 2023-01-29T01:54:54Z

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot · 2023-01-29T17:29:06Z

🔔 This is now entering its final comment period, as per the review above. 🔔

library/core/src/ffi/c_str.rs

joshtriplett · 2023-01-29T19:01:36Z

@tgross35 Per @oli-obk's comment, could you update the PR to stabilize the constness as well?

tgross35 · 2023-01-29T20:09:41Z

@joshtriplett I just replied to that comment, but I wasn’t able to do so because memchr isn’t const stable. Is there a way around that since it’s an internal call?

mejrs · 2023-01-30T22:58:01Z

@joshtriplett I just replied to that comment, but I wasn’t able to do so because memchr isn’t const stable. Is there a way around that since it’s an internal call?

It is const. See core::slice::memchr::memchr.

tgross35 · 2023-01-30T23:20:55Z

It is const. See core::slice::memchr::memchr.

Hm, you're right. But marking it const stable gets this failure https://github.com/rust-lang/rust/actions/runs/4049015645/jobs/6964924272#step:26:777 any clue why?

The slice index is also an issue but I think that could be worked around with unsafe from_raw_parts if needed

mejrs · 2023-01-30T23:28:21Z

I believe you need https://rustc-dev-guide.rust-lang.org/stability.html#rustc_allow_const_fn_unstable

tgross35 · 2023-01-31T04:54:58Z

Halfway there; #[rustc_allow_const_fn_unstable(const_slice_index)] fixed the complaint about slice indexing. But do you have any idea what the const feature flag for memchr would be @mejrs? Doesn't look feature gated in the repo, and slice_internals doesn't work

oli-obk · 2023-01-31T08:49:57Z

Ah this is an oddity of our const stability checks. You need to add a #[rustc_const_stable] attribute on memchr (and probably recursively on memchr_naive and memchr_aligned, too). Even if the function isn't stable, it must be const stable to be usable in a const stable function.

rfcbot · 2023-02-08T17:29:55Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

dtolnay · 2023-02-08T19:29:31Z

@bors r+

bors · 2023-02-08T19:29:34Z

📌 Commit 877e9f5 has been approved by dtolnay

It is now in the queue for this repository.

…bilization, r=dtolnay Stabilize feature `cstr_from_bytes_until_nul` This PR seeks to stabilize `cstr_from_bytes_until_nul`. Partially addresses rust-lang#95027 This function has only been on nightly for about 10 months, but I think it is simple enough that there isn't harm discussing stabilization. It has also had at least a handful of mentions on both the user forum and the discord, so it seems like it's already in use or at least known. This needs FCP still. Comment on potential discussion points: - eventual conversion of `CStr` to be a single thin pointer: this function will still be useful to provide a safe way to create a `CStr` after this change. - should this return a length too, to address concerns about the `CStr` change? I don't see it as being particularly useful, and it seems less ergonomic (i.e. returning `Result<(&CStr, usize), FromBytesUntilNulError>`). I think users that also need this length without the additional `strlen` call are likely better off using a combination of other methods, but this is up for discussion - `CString::from_vec_until_nul`: this is also useful, but it doesn't even have a nightly implementation merged yet. I propose feature gating that separately, as opposed to blocking this `CStr` implementation on that Possible alternatives: A user can use `from_bytes_with_nul` on a slice up to `my_slice[..my_slice.iter().find(|c| c == 0).unwrap()]`. However; that is significantly less ergonomic, and is a bit more work for the compiler to optimize compared the direct `memchr` call that this wraps. ## New stable API ```rs // both in core::ffi pub struct FromBytesUntilNulError(()); impl CStr { pub const fn from_bytes_until_nul( bytes: &[u8] ) -> Result<&CStr, FromBytesUntilNulError> } ``` cc `@ericseppanen` original author, `@Mark-Simulacrum` original reviewer, `@m-ou-se` brought up some issues on the thin pointer CStr `@rustbot` modify labels: +T-libs-api +needs-fcp

…bilization, r=dtolnay Stabilize feature `cstr_from_bytes_until_nul` This PR seeks to stabilize `cstr_from_bytes_until_nul`. Partially addresses rust-lang#95027 This function has only been on nightly for about 10 months, but I think it is simple enough that there isn't harm discussing stabilization. It has also had at least a handful of mentions on both the user forum and the discord, so it seems like it's already in use or at least known. This needs FCP still. Comment on potential discussion points: - eventual conversion of `CStr` to be a single thin pointer: this function will still be useful to provide a safe way to create a `CStr` after this change. - should this return a length too, to address concerns about the `CStr` change? I don't see it as being particularly useful, and it seems less ergonomic (i.e. returning `Result<(&CStr, usize), FromBytesUntilNulError>`). I think users that also need this length without the additional `strlen` call are likely better off using a combination of other methods, but this is up for discussion - `CString::from_vec_until_nul`: this is also useful, but it doesn't even have a nightly implementation merged yet. I propose feature gating that separately, as opposed to blocking this `CStr` implementation on that Possible alternatives: A user can use `from_bytes_with_nul` on a slice up to `my_slice[..my_slice.iter().find(|c| c == 0).unwrap()]`. However; that is significantly less ergonomic, and is a bit more work for the compiler to optimize compared the direct `memchr` call that this wraps. ## New stable API ```rs // both in core::ffi pub struct FromBytesUntilNulError(()); impl CStr { pub const fn from_bytes_until_nul( bytes: &[u8] ) -> Result<&CStr, FromBytesUntilNulError> } ``` cc ``@ericseppanen`` original author, ``@Mark-Simulacrum`` original reviewer, ``@m-ou-se`` brought up some issues on the thin pointer CStr ``@rustbot`` modify labels: +T-libs-api +needs-fcp

…mpiler-errors Rollup of 9 pull requests Successful merges: - rust-lang#107317 (Implement `AsFd` and `AsRawFd` for `Rc`) - rust-lang#107429 (Stabilize feature `cstr_from_bytes_until_nul`) - rust-lang#107713 (Extend `BYTE_SLICE_IN_PACKED_STRUCT_WITH_DERIVE`.) - rust-lang#107761 (Replace a command line flag with an env var to allow tools to initialize the tracing loggers at their own discretion) - rust-lang#107790 ( x.py fails all downloads that use a tempdir with snap curl rust-lang#107722) - rust-lang#107799 (correctly update goals in the cache) - rust-lang#107813 (Do not eagerly recover for bad `impl Trait` types in macros) - rust-lang#107817 (rustdoc: use svgo to shrink `wheel.svg`) - rust-lang#107819 (Set `rust-analyzer.check.invocationLocation` to `root`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

library/core/src/ffi/c_str.rs

hkBst · 2023-02-13T11:34:15Z

Is there any reason for misspelling null as nul in cstr_from_bytes_until_nul?

ChrisDenton · 2023-02-13T11:36:18Z

For consistency with the other already stabilized methods.

m-ou-se · 2023-02-13T14:09:50Z

It's somewhat common to use "nul" for chars and "null" for pointers.

Stargateur · 2023-07-01T03:25:07Z

the ascii table has always noted the \0 character NUL (null)

rustbot assigned cuviper Jan 28, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 28, 2023

rustbot added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 28, 2023

tgross35 mentioned this pull request Jan 28, 2023

Tracking Issue for CStr::from_bytes_until_nul #95027

Closed

3 tasks

rustbot assigned m-ou-se and unassigned cuviper Jan 28, 2023

tgross35 force-pushed the from-bytes-until-null-stabilization branch 2 times, most recently from 36956fc to c223206 Compare January 28, 2023 23:13

joshtriplett removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jan 29, 2023

oli-obk reviewed Jan 29, 2023

View reviewed changes

library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

tgross35 mentioned this pull request Feb 3, 2023

Stabilize const_cstr_methods #107624

Merged

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 8, 2023

dtolnay approved these changes Feb 8, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2023

dtolnay assigned dtolnay and unassigned m-ou-se Feb 8, 2023

compiler-errors mentioned this pull request Feb 9, 2023

Rollup of 8 pull requests #107821

Closed

This was referenced Feb 9, 2023

Rollup of 9 pull requests #107823

Closed

Rollup of 9 pull requests #107828

Merged

bors merged commit aee4570 into rust-lang:master Feb 9, 2023

rustbot added this to the 1.69.0 milestone Feb 9, 2023

tgross35 deleted the from-bytes-until-null-stabilization branch February 9, 2023 09:18

oli-obk reviewed Feb 9, 2023

View reviewed changes

library/core/src/ffi/c_str.rs Show resolved Hide resolved

apiraino removed the to-announce Announce this issue on triage meeting label Feb 16, 2023

tgross35 mentioned this pull request Aug 3, 2023

CStr should have a function like from_bytes_with_nul() that reads until the first '\0' #49107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize feature `cstr_from_bytes_until_nul` #107429

Stabilize feature `cstr_from_bytes_until_nul` #107429

tgross35 commented Jan 28, 2023 •

edited

Loading

rustbot commented Jan 28, 2023

rustbot commented Jan 28, 2023

cuviper commented Jan 28, 2023

joshtriplett commented Jan 29, 2023

rfcbot commented Jan 29, 2023 •

edited by m-ou-se

Loading

rfcbot commented Jan 29, 2023

joshtriplett commented Jan 29, 2023

tgross35 commented Jan 29, 2023

mejrs commented Jan 30, 2023

tgross35 commented Jan 30, 2023

mejrs commented Jan 30, 2023

This comment has been minimized.

tgross35 commented Jan 31, 2023

oli-obk commented Jan 31, 2023

rfcbot commented Feb 8, 2023

dtolnay commented Feb 8, 2023

bors commented Feb 8, 2023

hkBst commented Feb 13, 2023

ChrisDenton commented Feb 13, 2023

m-ou-se commented Feb 13, 2023

Stargateur commented Jul 1, 2023

Stabilize feature cstr_from_bytes_until_nul #107429

Stabilize feature cstr_from_bytes_until_nul #107429

Conversation

tgross35 commented Jan 28, 2023 • edited Loading

New stable API

rustbot commented Jan 28, 2023

rustbot commented Jan 28, 2023

cuviper commented Jan 28, 2023

joshtriplett commented Jan 29, 2023

rfcbot commented Jan 29, 2023 • edited by m-ou-se Loading

rfcbot commented Jan 29, 2023

joshtriplett commented Jan 29, 2023

tgross35 commented Jan 29, 2023

mejrs commented Jan 30, 2023

tgross35 commented Jan 30, 2023

mejrs commented Jan 30, 2023

This comment has been minimized.

tgross35 commented Jan 31, 2023

oli-obk commented Jan 31, 2023

rfcbot commented Feb 8, 2023

dtolnay commented Feb 8, 2023

bors commented Feb 8, 2023

hkBst commented Feb 13, 2023

ChrisDenton commented Feb 13, 2023

m-ou-se commented Feb 13, 2023

Stargateur commented Jul 1, 2023

Stabilize feature `cstr_from_bytes_until_nul` #107429

Stabilize feature `cstr_from_bytes_until_nul` #107429

tgross35 commented Jan 28, 2023 •

edited

Loading

rfcbot commented Jan 29, 2023 •

edited by m-ou-se

Loading