Closed tgross35 closed 3 months ago
I gated CStr::count_bytes()
's constness under this same flag since it depends on the same decision. See https://github.com/rust-lang/rust/pull/114443
This method was previously gated under
const_cstr_methods
, but was split off after discussion in #100291.
I think this is referring to https://github.com/rust-lang/rust/pull/107624#pullrequestreview-1283351043. I have updated the link.
There's an ongoing RFC for stabilizing that intrinsic (without exposing it as public API), so let's just let that RFC handle it
I infer that this is https://github.com/rust-lang/rfcs/pull/3352. According to https://github.com/rust-lang/rust/pull/107624#discussion_r1101911091, T-libs-api should not look into stabilizing the APIs tracked by this tracking issue until some clarity has been reached by T-lang on that RFC.
I have added a link to that RFC to the checklist in this issue.
That all sounds correct, thanks for the updates
Why is https://github.com/rust-lang/rfcs/pull/3352 listed as a blocker here? Using const_eval_select in stable const fn is fine, and in fact we already do that, e.g. from_bytes_with_nul_unchecked
. It's only a problem if the two code paths do not behave the same way.
Oh I see, this probably goes back to this discussion. So far we're only using const_eval_select to control assertions, this needs it to provide a const-time implementation for strlen
.
But still, I don't think that requires the RFC -- as Oli said, all that requires is to commit to is that we'll have some way of calling strlen
in the runtime path of a const fn
. There's a lot of ways to do that.
@rust-lang/libs-api, @rust-lang/lang: @rfcbot fcp merge
I propose stabilizing const
on this already existing stable function. All that requires us to commit to is that we'll have some way of calling strlen
in the runtime path of a const fn
. There's a lot of ways to do that.
impl CStr {
pub const unsafe fn from_ptr<'a>(ptr: *const c_char) -> &'a CStr;
}
Relatedly, I propose moving the const
stability of the currently unstable count_bytes
method over to https://github.com/rust-lang/rust/issues/114441 as a libs-api-only issue — we'll stabilize count_bytes
separately without taking another T-lang approval to make it const
.
impl CStr {
#[unstable]
pub const fn count_bytes(&self) -> usize;
}
The const
for this pair of functions has been tracked here together because one of them will be trivial and one of them will be interesting under both the current and planned implementation of CStr
. If &CStr
is a fat pointer then from_ptr
is interesting and count_bytes
is trivial. If &CStr
is a thin pointer then from_ptr
is trivial and count_bytes
is interesting. The part that is interesting basically looks the same either way, and that is:
For previous discussion of whether this is okay to stabilize as const
, please see https://github.com/rust-lang/rust/pull/107624#discussion_r1101466769 and https://github.com/rust-lang/rust/pull/107624#issuecomment-1586686716. I am including T-lang in this FCP to vet this as well. Ralf's comments in this issue suggest we should not have to wait for progress on https://github.com/rust-lang/rfcs/pull/3352 as a prerequisite for stabilizing const fn from_ptr
because regardless of what happens with that RFC, we will always be able to come up with some way for const_strlen
to delegate to an optimized strlen
at runtime and a straightforward const strlen
in const, having identical observable behavior other than performance.
Note that this feature would be the first time we stabilize a usage of const_eval_select
that is not just changing assertions. The previous use of const_eval_select
in a stable const fn is from_bytes_with_nul_unchecked
as Ralf pointed out.
Team member @dtolnay 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!
cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. See this document for info about what commands tagged team members can give me.
Ralf's comments in this issue suggest we should not have to wait for progress on https://github.com/rust-lang/rfcs/pull/3352 as a prerequisite for stabilizing const fn from_ptr because regardless of what happens with that RFC, we will always be able to come up with some way for const_strlen to delegate to an optimized strlen at runtime and a straightforward const strlen in const, having identical observable behavior other than performance.
Exactly. I think it's fine to commit to having such a mechanism available. This does not commit us to any particular shape it may take.
Note that this feature would be the first time we stabilize a usage of const_eval_select that is not just changing assertions.
:+1: from my side.
Cc @rust-lang/wg-const-eval
Note that this feature would be the first time we stabilize a usage of
const_eval_select
that is not just changing assertions.
To elaborate on this a bit — This is quoted from https://github.com/rust-lang/rust/pull/107624#discussion_r1101466769. The distinction being made here between CStr::from_bytes_with_nul_unchecked
(already stable const) vs CStr::from_ptr
(this FCP) is that if const_eval_select
vanishes with no replacement, then we could reasonably switch to using the current runtime impl of from_bytes_with_nul_unchecked
unconditionally, since it is already compatible with const and just detects less UB than the current const impl. But if const_eval_select
vanishes with no replacement then for from_ptr
there would be consequences beyond just worse assertions in UB callers, namely we would be without a way to preserve its current runtime performance.
Given that
const unsafe fn count_bytes(p: *const u8) -> usize {
let mut i = 0;
loop {
if *p.add(i) == 0 { return i }
i += 1;
}
}
compiles on stable, I don't see any lang concerns here.
Totally happy leaving this up to libs-api.
@rfcbot reviewed
@joshtriplett @m-ou-se @nikomatsakis @pnkfelix @tmandry reminder that there's a checkbox here waiting for you. :)
Agreed with @scottmcm, and I'm personally comfortable with saying we'll always have a way to make this performant.
@rfcbot reviewed
(Agreed, @tmandry, I'm also personally comfortable with it -- we could also always make it an intrinsic if needed, like memcmp
#114382, if for some reason it needed to stop using C_E_S.)
@rfcbot reviewed
:bell: This is now entering its final comment period, as per the review above. :bell:
@rustbot labels -I-lang-nominated
We discussed this in the lang call today. Everyone felt comfortable with it, and it's now in FCP.
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.
Feature gate:
#![feature(const_cstr_from_ptr)]
This is a tracking issue for using
CStr::from_ptr()
in a const context.This method was previously gated under
const_cstr_methods
, but was split off after discussion in https://github.com/rust-lang/rust/pull/107624#pullrequestreview-1283351043.Public API
Steps / History
Blocked on https://github.com/rust-lang/rfcs/pull/3352 (?)Unresolved Questions
Making this const currently depends on
const_eval_select
, which unstable. Ifconst_eval_select
becomes internally usable, we will be able to stabilize this.The other currently available option is to always use a const
strlen
, but this would come with a performance hit.Alternatively, whenever
CStr
becomes a thin pointer this can be const stabilized since it will have no internal operations.