following up on btoi removal

[fbstj]

following up on #1292 with some "cleanup" of the trait & macro. feel free to take what you want from this

I changed to the ZERO constant but I also played around with some other changes to the macro I don't know what is more correct/idiomatic tbh

[Byron]

Astonishing! Do more with less, and even with added docs ❤️.

[benmkw]

Just looking at this, what one could actually do is: remove all the generics and traits, just have two functions which parse into i64 und u64 and then do try_into at the call side for the target type.

[fbstj]

I've been attempting that but:

• to_signed currently uses to_unsigned for the unsigned conversions
• the tests which check for overflows don't get the 'nice' error if you externally try-from; which I suspect is the more important thing?

[Byron]

I think it's OK to keep these generics as from my experience, rustc doesn't really have a problem with it unless it gets really complicated. This usage certainly doesn't qualify. Also, the amount of code it would instantiate seems minimal at best.

To my mind, btoi now has the sweetspot in terms of size and capability, and part of me wonders if this should be contributed back to btoi to increase compile times everywhere - there seems to be no downside (maybe I am missing something).

[benmkw]

rustc doesn't really have a problem with it unless it gets really complicated. This usage certainly doesn't qualify. Also, the amount of code it would instantiate seems minimal at best.

Yeah that sounds right, although it can be shorter/ more readable.
The following is a quick pass on it although it would need to be reviewed for correctness and a way to differentiate under and overflow would be nice. I would not really propose to invest too much time into it, just found it interesting.

I was also wondering why num-traits has a build.rs anyway cause it should really be a light dependency I'd like to think but it seems it wants to keep it rust-num/num-traits#200.

diff --git a/gix-quote/src/ansi_c.rs b/gix-quote/src/ansi_c.rs
index 43856f899..58f246f26 100644
--- a/gix-quote/src/ansi_c.rs
+++ b/gix-quote/src/ansi_c.rs
@@ -91,7 +91,7 @@ pub fn undo(input: &BStr) -> Result<(Cow<'_, BStr>, usize), undo::Error> {
                                     .expect("impossible to fail as numbers match");
                                 let byte = gix_utils::btoi::to_unsigned_with_radix(&buf, 8)
                                     .map_err(|e| undo::Error::new(e, original))?;
-                                out.push(byte);
+                                out.push(byte.try_into().unwrap());
                                 input = &input[2..];
                                 consumed += 2;
                             }
diff --git a/gix-utils/src/btoi.rs b/gix-utils/src/btoi.rs
index c2cc5e28c..df41c6607 100644
--- a/gix-utils/src/btoi.rs
+++ b/gix-utils/src/btoi.rs
@@ -79,8 +79,18 @@ impl std::error::Error for ParseIntegerError {
 /// assert!(to_unsigned::<u8>(b"256").is_err()); // overflow
 /// ```
 #[track_caller]
-pub fn to_unsigned<I: MinNumTraits>(bytes: &[u8]) -> Result<I, ParseIntegerError> {
-    to_unsigned_with_radix(bytes, 10)
+pub fn to_unsigned<I: std::convert::TryFrom<u64>>(bytes: &[u8]) -> Result<I, ParseIntegerError>
+where
+    <I as std::convert::TryFrom<u64>>::Error: std::fmt::Debug,
+{
+    let unsigned = to_unsigned_with_radix(bytes, 10)?;
+    match unsigned.try_into() {
+        Ok(v) => Ok(v),
+        Err(e) => Err(ParseIntegerError {
+            // TODO distinguish under and overflow here
+            kind: ErrorKind::Overflow,
+        }),
+    }
 }
 
 /// Converts a byte slice in a given base to an integer. Signs are not allowed.
@@ -91,13 +101,13 @@ pub fn to_unsigned<I: MinNumTraits>(bytes: &[u8]) -> Result<I, ParseIntegerError
 ///
 /// * `bytes` is empty
 /// * not all characters of `bytes` are `0-9`, `a-z` or `A-Z`
-/// * not all characters refer to digits in the given `radix`
+/// * not all characters refer to digits in the given `base`
 /// * the number overflows `I`
 ///
 /// # Panics
 ///
-/// Panics if `radix` is not in the range `2..=36` (or in the pathological
-/// case that there is no representation of `radix` in `I`).
+/// Panics if `base` is not in the range `2..=36` (or in the pathological
+/// case that there is no representation of `base` in `I`).
 ///
 /// # Examples
 ///
@@ -106,22 +116,20 @@ pub fn to_unsigned<I: MinNumTraits>(bytes: &[u8]) -> Result<I, ParseIntegerError
 /// assert_eq!(Ok(255), to_unsigned_with_radix(b"ff", 16));
 /// assert_eq!(Ok(42), to_unsigned_with_radix(b"101010", 2));
 /// ```
-pub fn to_unsigned_with_radix<I: MinNumTraits>(bytes: &[u8], radix: u32) -> Result<I, ParseIntegerError> {
+pub fn to_unsigned_with_radix(bytes: &[u8], base: u32) -> Result<u64, ParseIntegerError> {
     assert!(
-        (2..=36).contains(&radix),
-        "radix must lie in the range 2..=36, found {radix}"
+        (2..=36).contains(&base),
+        "base must lie in the range 2..=36, found {base}"
     );
 
-    let base = I::from_u32(radix).expect("radix can be represented as integer");
-
     if bytes.is_empty() {
         return Err(ParseIntegerError { kind: ErrorKind::Empty });
     }
 
-    let mut result = I::ZERO;
+    let mut result: u64 = 0;
 
     for &digit in bytes {
-        let x = match char::from(digit).to_digit(radix).and_then(I::from_u32) {
+        let x = match char::from(digit).to_digit(base).and_then(|num| num.try_into().unwrap()) {
             Some(x) => x,
             None => {
                 return Err(ParseIntegerError {
@@ -129,7 +137,7 @@ pub fn to_unsigned_with_radix<I: MinNumTraits>(bytes: &[u8], radix: u32) -> Resu
                 })
             }
         };
-        result = match result.checked_mul(base) {
+        result = match result.checked_mul(base.into()) {
             Some(result) => result,
             None => {
                 return Err(ParseIntegerError {
@@ -137,7 +145,7 @@ pub fn to_unsigned_with_radix<I: MinNumTraits>(bytes: &[u8], radix: u32) -> Resu
                 })
             }
         };
-        result = match result.checked_add(x) {
+        result = match result.checked_add(x.into()) {
             Some(result) => result,
             None => {
                 return Err(ParseIntegerError {
@@ -181,8 +189,19 @@ pub fn to_unsigned_with_radix<I: MinNumTraits>(bytes: &[u8], radix: u32) -> Resu
 ///
 /// assert!(to_signed::<i32>(b" 42").is_err()); // leading space
 /// ```
-pub fn to_signed<I: MinNumTraits>(bytes: &[u8]) -> Result<I, ParseIntegerError> {
-    to_signed_with_radix(bytes, 10)
+pub fn to_signed<I: std::convert::TryFrom<i64>>(bytes: &[u8]) -> Result<I, ParseIntegerError>
+where
+    <I as std::convert::TryFrom<i64>>::Error: std::fmt::Debug,
+{
+    let signed = to_signed_with_radix(bytes, 10)?;
+
+    match signed.try_into() {
+        Ok(v) => Ok(v),
+        Err(e) => Err(ParseIntegerError {
+            // TODO distinguish under and overflow here
+            kind: ErrorKind::Overflow,
+        }),
+    }
 }
 
 /// Converts a byte slice in a given base to an integer.
@@ -197,14 +216,14 @@ pub fn to_signed<I: MinNumTraits>(bytes: &[u8]) -> Result<I, ParseIntegerError>
 /// * `bytes` has no digits
 /// * not all characters of `bytes` are `0-9`, `a-z`, `A-Z`, exluding an
 ///   optional leading sign
-/// * not all characters refer to digits in the given `radix`, exluding an
+/// * not all characters refer to digits in the given `base`, exluding an
 ///   optional leading sign
 /// * the number overflows or underflows `I`
 ///
 /// # Panics
 ///
-/// Panics if `radix` is not in the range `2..=36` (or in the pathological
-/// case that there is no representation of `radix` in `I`).
+/// Panics if `base` is not in the range `2..=36` (or in the pathological
+/// case that there is no representation of `base` in `I`).
 ///
 /// # Examples
 ///
@@ -214,97 +233,24 @@ pub fn to_signed<I: MinNumTraits>(bytes: &[u8]) -> Result<I, ParseIntegerError>
 /// assert_eq!(Ok(10), to_signed_with_radix(b"+a", 16));
 /// assert_eq!(Ok(-42), to_signed_with_radix(b"-101010", 2));
 /// ```
-pub fn to_signed_with_radix<I: MinNumTraits>(bytes: &[u8], radix: u32) -> Result<I, ParseIntegerError> {
+pub fn to_signed_with_radix(bytes: &[u8], base: u32) -> Result<i64, ParseIntegerError> {
     assert!(
-        (2..=36).contains(&radix),
-        "radix must lie in the range 2..=36, found {radix}"
+        (2..=36).contains(&base),
+        "base must lie in the range 2..=36, found {base}"
     );
 
-    let base = I::from_u32(radix).expect("radix can be represented as integer");
-
     if bytes.is_empty() {
         return Err(ParseIntegerError { kind: ErrorKind::Empty });
     }
 
-    let digits = match bytes[0] {
-        b'+' => return to_unsigned_with_radix(&bytes[1..], radix),
-        b'-' => &bytes[1..],
-        _ => return to_unsigned_with_radix(bytes, radix),
-    };
-
-    if digits.is_empty() {
-        return Err(ParseIntegerError { kind: ErrorKind::Empty });
-    }
-
-    let mut result = I::ZERO;
-
-    for &digit in digits {
-        let x = match char::from(digit).to_digit(radix).and_then(I::from_u32) {
-            Some(x) => x,
-            None => {
-                return Err(ParseIntegerError {
-                    kind: ErrorKind::InvalidDigit,
-                })
-            }
-        };
-        result = match result.checked_mul(base) {
-            Some(result) => result,
-            None => {
-                return Err(ParseIntegerError {
-                    kind: ErrorKind::Underflow,
-                })
-            }
-        };
-        result = match result.checked_sub(x) {
-            Some(result) => result,
-            None => {
-                return Err(ParseIntegerError {
-                    kind: ErrorKind::Underflow,
-                })
-            }
-        };
-    }
-
-    Ok(result)
-}
-
-/// minimal subset of traits used by [`to_signed_with_radix`] and [`to_unsigned_with_radix`]
-pub trait MinNumTraits: Sized + Copy + TryFrom<u32> {
-    /// the 0 value for this type
-    const ZERO: Self;
-    /// convert from a unsinged 32-bit word
-    fn from_u32(n: u32) -> Option<Self> {
-        Self::try_from(n).ok()
-    }
-    /// the checked multiplication operation for this type
-    fn checked_mul(self, rhs: Self) -> Option<Self>;
-    /// the chekced addition operation for this type
-    fn checked_add(self, rhs: Self) -> Option<Self>;
-    /// the checked subtraction operation for this type
-    fn checked_sub(self, v: Self) -> Option<Self>;
-}
-
-macro_rules! impl_checked {
-    ($f:ident) => {
-        fn $f(self, rhs: Self) -> Option<Self> {
-            Self::$f(self, rhs)
+    match bytes[0] {
+        b'+' => return to_unsigned_with_radix(&bytes[1..], base).map(|num| num.try_into().unwrap()),
+        b'-' => {
+            return to_unsigned_with_radix(&bytes[1..], base).map(|num| {
+                let signed: i64 = num.try_into().unwrap();
+                -signed
+            })
         }
-    };
-}
-
-macro_rules! min_num_traits {
-    ($t:ty) => {
-        impl MinNumTraits for $t {
-            const ZERO: Self = 0;
-            impl_checked!(checked_add);
-            impl_checked!(checked_mul);
-            impl_checked!(checked_sub);
-        }
-    };
+        _ => return to_unsigned_with_radix(bytes, base).map(|num| num.try_into().unwrap()),
+    }
 }
-
-min_num_traits!(i32);
-min_num_traits!(i64);
-min_num_traits!(u64);
-min_num_traits!(u8);
-min_num_traits!(usize);

[Byron]

Quite amazing how much less code is possible here. But I also agree, what's here now isn't bad at all, compiles reasonably fast and doesn't take shortcuts when dealing with errors. So let's keep the current version (I guess ).