Get rid of `EscapeDebugInner`. #138237

reitermarkus · 2025-03-08T20:14:13Z

I read the note on EscapeDebugInner and thought I'd give it a try.

rustbot · 2025-03-08T20:14:17Z

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

hkBst · 2025-03-09T15:17:24Z

library/core/src/char/mod.rs

-// Note: It’s possible to manually encode the EscapeDebugInner inside of
-// EscapeIterInner (e.g. with alive=254..255 indicating that data[0..4] holds
-// a char) which would likely result in a more optimised code.  For now we use
-// the option easier to implement.


I think this is the comment you reference, so perfrun seems to be in order.

hkBst · 2025-03-09T15:19:23Z

library/core/src/escape.rs

+}
+
+/// Marker type to indicate that the character is always escaped,
+/// use to optimized the iterator implementation.


used to optimize?

hkBst · 2025-03-09T15:19:32Z

library/core/src/escape.rs

+pub(crate) struct AlwaysEscaped;
+
+/// Marker type to indicate that the character may be escaped,
+/// use to optimized the iterator implementation.


used to optimize?

hkBst · 2025-03-09T15:20:41Z

library/core/src/escape.rs

+#[derive(Clone, Copy)]
+#[non_exhaustive]
+pub(crate) struct MaybeEscaped;
+
 /// An iterator over an fixed-size array.


might as well fix an to a here

hkBst · 2025-03-09T15:24:00Z

library/core/src/escape.rs

+    // Invariant: For non-printable characters, `alive.start <= alive.end <= N`,
+    // for printable characters, `alive.end >= alive.start >= 128`.


Seems more clear to me if this used <= throughout

hkBst · 2025-03-09T15:33:10Z

library/core/src/escape.rs

+    /// The caller must ensure that `self` contains an escape sequence.
    #[inline]
-    pub(crate) fn as_ascii(&self) -> &[ascii::Char] {
-        // SAFETY: `self.alive` is guaranteed to be a valid range for indexing `self.data`.
+    unsafe fn as_ascii(&self) -> &[ascii::Char] {
+        // SAFETY: `self.data.escaped` contains an escape sequence, as is guaranteed
+        // by the caller, and `self.alive` is guaranteed to be a valid range for


It seems to me that the top level safety comment should state the more detailed requirement that callers are expected to uphold, and then the inner unsafe use can say that it relies on that.

Tangentially, I'm wondering if escaped would not be better as escape_seq or escape.

hkBst · 2025-03-09T15:38:14Z

library/core/src/escape.rs

+union MaybeEscapedCharacter<const N: usize> {
+    pub escaped: [ascii::Char; N],
+    pub unescaped: char,


Seems to me like the variants should be called something closer to escape_seq/escape and literal/lit.

hkBst · 2025-03-09T15:40:43Z

library/core/src/escape.rs

+    /// The caller must ensure that `self` contains an unescaped `char`.
    #[inline]
-    pub(crate) fn as_str(&self) -> &str {
-        self.as_ascii().as_str()
+    unsafe fn as_char(&self) -> char {
+        // SAFETY: `self.data.unescaped` contains an unescaped `char`,


Instead of "unescaped char", this might be clearer as "literal char".

hkBst · 2025-03-09T15:49:27Z

library/core/src/escape.rs

+impl<const N: usize> EscapeIterInner<N, AlwaysEscaped> {
    pub(crate) fn next(&mut self) -> Option<u8> {
        let i = self.alive.next()?;

-        // SAFETY: `i` is guaranteed to be a valid index for `self.data`.
-        unsafe { Some(self.data.get_unchecked(usize::from(i)).to_u8()) }
+        // SAFETY: We just checked that `self` contains an escape sequence, and
+        // `i` is guaranteed to be a valid index for `self.data.escaped`.
+        unsafe { Some(self.data.escaped.get_unchecked(usize::from(i)).to_u8()) }


I don't understand the safety comment as written (and checked whether "let i = self.alive.next()?;" does any such checking, but no, it is just a Range of hopefully valid indices for the escape seq.). Shouldn't this refer to the AlwaysEscaped type state instead?

hkBst · 2025-03-09T15:52:13Z

library/core/src/escape.rs

-    pub(crate) fn advance_by(&mut self, n: usize) -> Result<(), NonZero<usize>> {
-        self.alive.advance_by(n)
+impl<const N: usize> EscapeIterInner<N, MaybeEscaped> {
+    const CHAR_FLAG: u8 = 0b10000000;


either add an underscore in the middle or use hex: 0x80

hkBst · 2025-03-09T15:57:46Z

library/core/src/escape.rs

+    // This is the only way to create an `EscapeIterInner` with an unescaped `char`, which
+    // means the `AlwaysEscaped` marker guarantees that `self` contains an escape sequence.


If this is the only way to create an EscapeIterInner<N, MaybeEscaped> with a literal char in its union member, then I don't see how that guarantees anything about the contents of EscapeIterInner<N, AlwaysEscaped>, besides that the union in an EscapeIterInner<N, AlwaysEscaped> contains a [ascii::Char; N] which may or may not be a valid escape sequence...

hkBst · 2025-03-09T16:04:54Z

library/core/src/escape.rs

    #[inline]
-    pub(crate) fn as_ascii(&self) -> &[ascii::Char] {
-        // SAFETY: `self.alive` is guaranteed to be a valid range for indexing `self.data`.
+    unsafe fn as_ascii(&self) -> &[ascii::Char] {


A more meaningful name seems to me as_escape or as_escape_seq

hkBst · 2025-03-09T16:12:03Z

library/core/src/escape.rs

+
+        if self.is_unescaped() {
+            // SAFETY: We just checked that `self` contains an unescaped `char`.
+            return Some(unsafe { self.as_char() });


maybe just inline as_char?

rustbot assigned thomcc Mar 8, 2025

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 Mar 8, 2025

reitermarkus force-pushed the remove-escape-debug-inner branch 2 times, most recently from e1c078d to 57c0a80 Compare March 8, 2025 20:35

Get rid of EscapeDebugInner.

0854482

reitermarkus force-pushed the remove-escape-debug-inner branch from 57c0a80 to 0854482 Compare March 8, 2025 20:52

hkBst reviewed Mar 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of `EscapeDebugInner`. #138237

Get rid of `EscapeDebugInner`. #138237

reitermarkus commented Mar 8, 2025

rustbot commented Mar 8, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

hkBst Mar 9, 2025

		// Invariant: For non-printable characters, `alive.start <= alive.end <= N`,
		// for printable characters, `alive.end >= alive.start >= 128`.

		// This is the only way to create an `EscapeIterInner` with an unescaped `char`, which
		// means the `AlwaysEscaped` marker guarantees that `self` contains an escape sequence.

Get rid of EscapeDebugInner. #138237

Are you sure you want to change the base?

Get rid of EscapeDebugInner. #138237

Conversation

reitermarkus commented Mar 8, 2025

rustbot commented Mar 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Get rid of `EscapeDebugInner`. #138237

Get rid of `EscapeDebugInner`. #138237