"Benno Lossin" <los...@kernel.org> writes:
> On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>> I'm having a difficult time parsing. Are you suggesting that we guard
>> against implementations of `TryInto<u64>` that misbehave?
>
> Let me try a different explanation:
>
> The safety requirement for implementing the `FromStrRadix`:
>
> /// The member functions of this trait must be implemented according to
> /// their documentation.
>
> Together with the functions of the trait:
>
> /// Parse `src` to [`Self`] using radix `radix`.
> fn from_str_radix(src: &BStr, radix: u32) -> Result<Self,
> crate::error::Error>;
>
> /// Return the absolute value of [`Self::MIN`].
> fn abs_min() -> u64;
>
> /// Perform bitwise 2's complement on `self`.
> ///
> /// Note: This function does not make sense for unsigned integers.
> fn complement(self) -> Self;
>
> Doesn't make sense. What does it mean to return the "absolute value of
> [`Self::MIN`]"? We don't have "absolute value" defined for an arbitrary
> type. Similarly for `complement` and `from_str_radix`, what does "Parse
> `src` to [`Self`] using radex `radix`" mean? It's not well-defined.
>
> You use this safety requirement in the parsing branch for negative
> numbers (the `unsafe` call at the bottom):
>
> [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())
> }
>
> But you don't mention that the check is valid due to the safety
> requirements of implementing `FromStrRadix`. But even if you did, that
> wouldn't mean anything as I explained above.
>
> So let's instead move all of this negation & u64 conversion logic into
> the `FromStrRadix` trait. Then it can be safe & the `ParseInt::from_str`
> function doesn't use `unsafe` (there still will be `unsafe` in the
> macro, but that is fine, as it's more local and knows the concrete
> types).
>
Alright. I guess my safety comments are slightly hand-wavy. Thanks for
the suggestion, I'll apply that for next spin.
Best regards,
Andreas Hindborg
