Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use TryFrom traits for utility types such as BitWidth #41

Open
Tracked by #46
Robbepop opened this issue Jul 14, 2019 · 3 comments
Open
Tracked by #46

Use TryFrom traits for utility types such as BitWidth #41

Robbepop opened this issue Jul 14, 2019 · 3 comments

Comments

@Robbepop
Copy link
Owner

No description provided.

@AaronKutch
Copy link
Contributor

I have been trying to rework the way BitWidth is constructed, and have almost done it on my tryfrom_construction branch (note: I forgot to make a new branch from master after the previous PR got squashed, so I will need to redo this branch before it can be PRed). The new way things are done is through these:

pub struct BitWidth(NonZeroUsize);

impl From<NonZeroUsize> for BitWidth {
    /// Creates a `BitWidth` from the given `NonZeroUsize`.
    fn from(width: NonZeroUsize) -> Self {
        BitWidth(width)
    }
}

impl TryFrom<usize> for BitWidth {
    type Error = Error; // This is the crates `Error` type

    /// Creates a `BitWidth` from the given `usize`.
    ///
    /// # Errors
    ///
    /// - If the given `width` is equal to zero.
    fn try_from(width: usize) -> Result<Self> {
        match NonZeroUsize::new(width) {
            Some(bitwidth) => Ok(BitWidth(bitwidth)),
            None => Err(Error::invalid_zero_bitwidth()),
        }
    }
}

pub(crate) fn bw<S>(width: S) -> BitWidth
where
    S: TryInto<BitWidth>,
{
    // For this case, we erase the regular error unwrapping message by converting
    // the `Result` to an `Option`, and displaying a different message.
    width.try_into().ok().expect(
        "Tried to construct an invalid BitWidth of 0 using the `apint::bw` function",
    )
}

To start off, I make BitWidth a wrapper around NonZeroUsize in case of compiler optimizations, but I do not expose this outside the crate.
The bw function (I used to want to make it a macro, but macros are difficult deal with when exporting and importing) acts as a general purpose way of quickly making constant bitwidths. I think I like it enough that we should make it public right away. It replaces the BitWidth::wX() functions will help greatly with testing and using the crate in practice.
The TryFrom impl causes the most changes. For example, verify_valid_bitwidth is removed in favor of BitWidth::try_from()? and signatures are changed from
W: Into<BitWidth> into W: TryInto<BitWidth, Error = Error>.
This enables directly inputing constants into the functions which is a massive ergonomics increase for users of the crate.

I encountered a huge problem, however. When I change the signatures:

pub fn truncate<W>(&mut self, target_width: W) -> Result<()>
    where
-        W: Into<BitWidth>,
+        W: TryInto<BitWidth, Error = Error>, // `Error = Error` is to set the return type to the crates `Error` type, or else it fails to compile
    {
        let actual_width = self.width();
-        let target_width = target_width.into();
+        let target_width = target_width.try_into()?;

rustc no longer accepts BitWidths directly as arguments. The workaround is to call .to_usize():

            // `excess_width` assigned to earlier
            self.most_significant_digit_mut()
-                 .truncate_to(excess_width)
+                .truncate_to(excess_width.to_usize())
                .expect(
                    "Excess bits are guaranteed to be within the bounds for valid \
                     truncation of a single `Digit`.",

The reason is that there is a blanket impl for TryFrom<T> for <T> that has a type Error = ! (the never type, or on stable Infallible. This will conflict with the requirement that Error = Error

error[E0271]: type mismatch resolving `<bitwidth::BitWidth as std::convert::TryInto<bitwidth::BitWidth>>::Error == errors::Error`                                                                
   --> src\apint\casting.rs:182:18
    |
182 |                 .truncate_to(excess_width)
    |                  ^^^^^^^^^^^ expected enum `std::convert::Infallible`, found struct `errors::Error`

@AaronKutch
Copy link
Contributor

I have finally fixed the problem. My only concern is that the generic function will be compiled into many functions, one for each kind of input. In the worst case, I can just have the generic function call an inner function, and the compiler will certainly inline the outer part into caller code. I think we already do that for the into_ functions through try_forward_bin_mut_impl. Does this look good?

@AaronKutch
Copy link
Contributor

Have you read this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants