Re: [PATCH] rust: str: implement `Display` and `Debug` for `BStr`

[Yutaro Ohno <yutaro.ono.418@gmail.com>]

On Wed, Jan 24, 2024 at 08:18:32PM +0000, Gary Guo wrote:
> On Thu, 25 Jan 2024 01:58:05 +0900
> Yutaro Ohno <yutaro.ono.418@gmail.com> wrote:
> 
> > Currently, `BStr` is just a type alias of `[u8]`, limiting its
> > representation to a byte list rather than a character list, which is not
> > ideal for printing and debugging.
> > 
> > Implement `Display` and `Debug` traits for `BStr` to facilitate easier
> > printing and debugging.
> > 
> > Also, for this purpose, change `BStr` from a type alias of `[u8]` to a
> > struct wrapper of `[u8]`.
> > 
> > Co-developed-by: Virgile Andreani <armavica@ulminfo.fr>
> > Signed-off-by: Virgile Andreani <armavica@ulminfo.fr>
> > Signed-off-by: Yutaro Ohno <yutaro.ono.418@gmail.com>
> > ---
> >  rust/kernel/str.rs | 211 +++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 186 insertions(+), 25 deletions(-)
> > 
> > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> > index 7d848b83add4..0f0261e063d2 100644
> > --- a/rust/kernel/str.rs
> > +++ b/rust/kernel/str.rs
> > @@ -14,8 +14,104 @@
> >  
> >  /// Byte string without UTF-8 validity guarantee.
> >  ///
> > -/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
> > -pub type BStr = [u8];
> > +/// `BStr` is simply a wrapper over `[u8]`, but has a more evident semantical
> > +/// meaning.
> 
> I think this line can go?
> 

Indeed. Will remove in v2.

> > +#[repr(transparent)]
> > +pub struct BStr([u8]);
> > +
> > +impl BStr {
> > +    /// Returns the length of this string.
> > +    #[inline]
> > +    pub const fn len(&self) -> usize {
> > +        self.0.len()
> > +    }
> > +
> > +    /// Returns `true` if the string is empty.
> > +    #[inline]
> > +    pub const fn is_empty(&self) -> bool {
> > +        self.len() == 0
> > +    }
> > +
> > +    /// Creates a [`BStr`] from a `[u8]`.
> > +    #[inline]
> > +    pub const fn from_bytes(bytes: &[u8]) -> &Self {
> > +        // SAFETY: BStr is transparent to [u8].
> > +        unsafe { &*(bytes as *const [u8] as *const BStr) }
> > +    }
> > +}
> > +
> > +impl fmt::Display for BStr {
> > +    /// Formats printable ASCII characters, escaping the rest.
> > +    ///
> > +    /// ```
> > +    /// # use kernel::{fmt, b_str, str::{BStr, CString}};
> > +    /// let ascii = b_str!("Hello, BStr!");
> > +    /// let s = CString::try_from_fmt(fmt!("{}", ascii)).unwrap();
> > +    /// assert_eq!(s.as_bytes(), "Hello, BStr!".as_bytes());
> > +    ///
> > +    /// let non_ascii = b_str!("🦀");
> > +    /// let s = CString::try_from_fmt(fmt!("{}", non_ascii)).unwrap();
> > +    /// assert_eq!(s.as_bytes(), "\\xf0\\x9f\\xa6\\x80".as_bytes());
> > +    /// ```
> > +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> > +        for &b in &self.0 {
> > +            match b {
> > +                // Common escape codes.
> > +                b'\t' => f.write_str("\\t")?,
> > +                b'\n' => f.write_str("\\n")?,
> > +                b'\r' => f.write_str("\\r")?,
> 
> The current CStr code will print these as is. Why escaping these for
> Display? Also, if you want to change the behaviour, please put it in a
> separate patch.
> 

That makes sense. I'll change.

> > +                // Printable characters.
> > +                0x20..=0x7e => f.write_char(b as char)?,
> > +                _ => write!(f, "\\x{:02x}", b)?,
> > +            }
> > +        }
> > +        Ok(())
> > +    }
> > +}
> > +
> > +impl fmt::Debug for BStr {
> > +    /// Formats printable ASCII characters with a double quote on either end,
> > +    /// escaping the rest.
> > +    ///
> > +    /// ```
> > +    /// # use kernel::{fmt, b_str, str::{BStr, CString}};
> > +    /// // Embedded double quotes are escaped.
> > +    /// let ascii = b_str!("Hello, \"BStr\"!");
> > +    /// let s = CString::try_from_fmt(fmt!("{:?}", ascii)).unwrap();
> > +    /// assert_eq!(s.as_bytes(), "\"Hello, \\\"BStr\\\"!\"".as_bytes());
> > +    ///
> > +    /// let non_ascii = b_str!("😺");
> > +    /// let s = CString::try_from_fmt(fmt!("{:?}", non_ascii)).unwrap();
> > +    /// assert_eq!(s.as_bytes(), "\"\\xf0\\x9f\\x98\\xba\"".as_bytes());
> > +    /// ```
> > +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> > +        f.write_str("\"")?;
> > +        for &b in &self.0 {
> > +            match b {
> > +                // Common escape codes.
> > +                b'\t' => f.write_str("\\t")?,
> > +                b'\n' => f.write_str("\\n")?,
> > +                b'\r' => f.write_str("\\r")?,
> > +                // String escape characters.
> > +                b'\\' => f.write_str("\\\\")?,
> > +                b'\"' => f.write_str("\\\"")?,
> > +                // Printable characters.
> > +                0x20..=0x7e => f.write_char(b as char)?,
> > +                _ => write!(f, "\\x{:02x}", b)?,
> > +            }
> > +        }
> > +        f.write_str("\"")
> > +    }
> > +}
> > +
> > +impl Deref for BStr {
> > +    type Target = [u8];
> > +
> > +    #[inline]
> > +    fn deref(&self) -> &Self::Target {
> > +        &self.0
> > +    }
> > +}
> >  
> >  /// Creates a new [`BStr`] from a string literal.
> >  ///
> > @@ -33,7 +129,7 @@
> >  macro_rules! b_str {
> >      ($str:literal) => {{
> >          const S: &'static str = $str;
> > -        const C: &'static $crate::str::BStr = S.as_bytes();
> > +        const C: &'static $crate::str::BStr = BStr::from_bytes(S.as_bytes());
> >          C
> >      }};
> >  }
> > @@ -225,15 +321,7 @@ impl fmt::Display for CStr {
> >      /// assert_eq!(s.as_bytes_with_nul(), "so \"cool\"\0".as_bytes());
> >      /// ```
> >      fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> > -        for &c in self.as_bytes() {
> > -            if (0x20..0x7f).contains(&c) {
> > -                // Printable character.
> > -                f.write_char(c as char)?;
> > -            } else {
> > -                write!(f, "\\x{:02x}", c)?;
> > -            }
> > -        }
> > -        Ok(())
> > +        fmt::Display::fmt(self.as_ref(), f)
> >      }
> >  }
> >  
> > @@ -255,23 +343,14 @@ impl fmt::Debug for CStr {
> >      /// assert_eq!(s.as_bytes_with_nul(), "\"so \\\"cool\\\"\"\0".as_bytes());
> >      /// ```
> >      fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> > -        f.write_str("\"")?;
> > -        for &c in self.as_bytes() {
> > -            match c {
> > -                // Printable characters.
> > -                b'\"' => f.write_str("\\\"")?,
> > -                0x20..=0x7e => f.write_char(c as char)?,
> > -                _ => write!(f, "\\x{:02x}", c)?,
> > -            }
> > -        }
> > -        f.write_str("\"")
> > +        fmt::Debug::fmt(self.as_ref(), f)
> >      }
> >  }
> >  
> >  impl AsRef<BStr> for CStr {
> >      #[inline]
> >      fn as_ref(&self) -> &BStr {
> > -        self.as_bytes()
> > +        BStr::from_bytes(self.as_bytes())
> >      }
> >  }
> >  
> > @@ -280,7 +359,7 @@ impl Deref for CStr {
> >  
> >      #[inline]
> >      fn deref(&self) -> &Self::Target {
> > -        self.as_bytes()
> > +        BStr::from_bytes(self.as_bytes())
> 
> Either use `as_ref` here or use `deref()` in the `as_ref`.
> 

I'll change in v2.

> >      }
> >  }
> >  
> > @@ -327,7 +406,7 @@ impl<Idx> Index<Idx> for CStr
> >  
> >      #[inline]
> >      fn index(&self, index: Idx) -> &Self::Output {
> > -        &self.as_bytes()[index]
> > +        &self.as_ref()[index]
> >      }
> >  }
>