Re: [PATCH v12 1/3] rust: str: add radix prefixed integer parsing functions

Benno Lossin
2025-05-07T08:58:15Z

On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 878111cb77bc..174e70397305 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -573,7 +573,6 @@ macro_rules! c_str {
> }
>
> #[cfg(test)]
> -#[expect(clippy::items_after_test_module)]
> mod tests {
> use super::*;
>
> @@ -946,3 +945,174 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> macro_rules! fmt {
> ($($f:tt)*) => ( core::format_args!($($f)*) )
> }
> +
> +/// Integer parsing functions for parsing signed and unsigned integers
> +/// potentially prefixed with `0x`, `0o`, or `0b`.
> +pub mod parse_int {

Why not make this its own file? It's 172 lines long already.

> + pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
> + /// Parse a string according to the description in [`Self`].
> + fn from_str(src: &BStr) -> Result<Self> {
> + match src.deref() {
> + [b'-', rest @ ..] => {
> + let (radix, digits) = strip_radix(rest.as_ref());
> + // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
> + // So if we want to parse negative numbers as positive and
> + // later multiply by -1, we have to parse into a larger
> + // integer. We choose `u64` as sufficiently large.
> + //
> + // NOTE: 128 bit integers are not available on all
> + // platforms, hence the choice of 64 bits.
> + let val = u64::from_str_radix(
> + core::str::from_utf8(digits).map_err(|_| EINVAL)?,
> + radix,
> + )
> + .map_err(|_| EINVAL)?;
> +
> + if val > Self::abs_min() {
> + return Err(EINVAL);
> + }
> +
> + if val == Self::abs_min() {
> + return Ok(Self::MIN);
> + }
> +
> + // SAFETY: We checked that `val` will fit in `Self` above.
> + let val: Self = unsafe { val.try_into().unwrap_unchecked() };
> +
> + Ok(val.complement())

You're allowing to parse `u32` with a leading `-`? I'd expect an error
in that case. Maybe `complement` should be named `negate` and return a
`Result`?

---
Cheers,
Benno

> + }
> + _ => {
> + let (radix, digits) = strip_radix(src);
> + Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
> + }
> + }
> + }
> + }