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

["Benno Lossin" <lossin@kernel.org>]

On Wed May 7, 2025 at 11:15 AM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
>>> +    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`?
>
> You would get `Err(EINVAL)` in that case, hitting this:
>
>   if val > Self::abs_min() {
>       return Err(EINVAL);
>   }

Ah, I think I asked this in a previous version already... Thanks.

---
Cheers,
Benno