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

Stabilize const_cstr_from_ptr (CStr::from_ptr, CStr::count_bytes) #127433

Merged
merged 2 commits into from
Jul 12, 2024
Merged

Stabilize const_cstr_from_ptr (CStr::from_ptr, CStr::count_bytes) #127433

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
53d3e62
Stabilize const_cstr_from_ptr (CStr::from_ptr, CStr::count_bytes)
dtolnay Jul 6, 2024
7f1518b
Add instability attribute on private const_strlen function
dtolnay Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions library/core/src/ffi/c_str.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,6 @@ impl CStr {
/// ```
///
/// ```
/// #![feature(const_cstr_from_ptr)]
///
/// use std::ffi::{c_char, CStr};
///
/// const HELLO_PTR: *const c_char = {
Expand All @@ -280,7 +278,7 @@ impl CStr {
#[inline] // inline is necessary for codegen to see strlen.
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_cstr_from_ptr", issue = "113219")]
#[rustc_const_stable(feature = "const_cstr_from_ptr", since = "CURRENT_RUSTC_VERSION")]
pub const unsafe fn from_ptr<'a>(ptr: *const c_char) -> &'a CStr {
// SAFETY: The caller has provided a pointer that points to a valid C
// string with a NUL terminator less than `isize::MAX` from `ptr`.
Expand Down Expand Up @@ -542,7 +540,7 @@ impl CStr {
#[must_use]
#[doc(alias("len", "strlen"))]
#[stable(feature = "cstr_count_bytes", since = "1.79.0")]
#[rustc_const_unstable(feature = "const_cstr_from_ptr", issue = "113219")]
#[rustc_const_stable(feature = "const_cstr_from_ptr", since = "CURRENT_RUSTC_VERSION")]
pub const fn count_bytes(&self) -> usize {
self.inner.len() - 1
}
Expand Down Expand Up @@ -742,6 +740,9 @@ impl AsRef<CStr> for CStr {
/// The pointer must point to a valid buffer that contains a NUL terminator. The NUL must be
/// located within `isize::MAX` from `ptr`.
#[inline]
#[unstable(feature = "cstr_internals", issue = "none")]
#[rustc_const_stable(feature = "const_cstr_from_ptr", since = "CURRENT_RUSTC_VERSION")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming the attribute on a nonpublic function allows using const_strlen everywhere without adding const_eval_select each time?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not to add such misuses of stability attributes. It makes it harder to review internal changes, because an important part of T-libs reviews when you aren't reviewing an addition API is making sure that you are not, in fact, adding API. This weakens such a defense and makes it easier for a reviewer to get confused.

Copy link
Contributor

Choose a reason for hiding this comment

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

In what way is this a misuse? The lang and libs teams FCPd having a const_strlen available in some form for internal use, currently via const_eval_select but that is an implementation detail. Marking it as "stable" (but of course nonpublic) seems to accurately reflect that status, as opposed to leaking the const_eval_select implementation detail around everywhere it is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right @tgross35, I believe this is const stability working as intended. The attribute enforces that future changes to this function do not introduce any unapproved implementation, because this private function now participates in the implementation of some public stable const function and the use of any new weird const internals here would become a stable commitment. Flagging the presence of a stable commitment is the point of a stability attribute.

@RalfJung endorsed the same thing recently in #124941 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be okay to rename const_strlen to just strlen at some point to make it more clear that's just our implementation of strlen now (avoiding confusion like #127444).

Copy link
Member

Choose a reason for hiding this comment

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

I believe it should have a comment disclaiming its status, then, rather than passing unremarked, because this strikes me as a nonintuitive combination of attributes when placed in a public module.

Or it could be marked #[unstable], as the other function cited is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added unstable in 7f1518b.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. "Fix this one attribute" or "oops this stabilization missed a spot" or "make this thing public that obviously was intended to be public... right?" are all fairly common PRs.

#[rustc_allow_const_fn_unstable(const_eval_select)]
const unsafe fn const_strlen(ptr: *const c_char) -> usize {
const fn strlen_ct(s: *const c_char) -> usize {
let mut len = 0;
Expand Down
Loading