Closed AaronKutch closed 4 years ago
Merging #58 into master will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master #58 +/- ##
=======================================
Coverage 81.87% 81.87%
=======================================
Files 23 23
Lines 5132 5132
=======================================
Hits 4202 4202
Misses 930 930
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 2818e58...e0f0287. Read the comment docs.
I realized that I should make better documentation, does the documentation explain the changes to you well?
I have two months left now before university starts up again
does this documentation make sense? This is in the last commit:
/// /// # Examples /// /// Most of what the functions in this library can do is self explanatory /// through the extensive function documentation, however there are details from /// across the library that need to be brought together when making things less /// verbose. /// ///
/// prelude::*,
/// Result,
/// };
///
/// use core::convert::TryFrom;
///
/// // This is just a dummy example, practical functions normally use `BitWidth`s as bit
/// // width arguments or have `TryInto<BitWidth>` based signatures like the one
/// // `ApInt::extend` uses.
/// fn example_function(input_width: usize) -> Result<ApInt> {
/// // The standard way of constructing `BitWidth`s and propogating errors up the call
/// // stack if the input is not a valid `BitWidth`.
/// let width = BitWidth::try_from(input_width)?;
///
/// // most single argument functions do not need any kind of error handling because
/// // the invariants are already handled by `BitWidth`.
/// let mut x = ApInt::signed_max_value(width);
/// x.sign_resize(width);
/// x.zero_resize(width);
///
/// // Let us resize `x` to some constant size. When using a constant or literal, a
/// // better shorthand for `BitWidth::try_from(42)?` is to use the `bw` free function
/// // instead:
/// x.sign_resize(bw(42));
///
/// // Let us extend `x` to another constant size. In this case, even though the
/// // `BitWidth` invariant is already handled, there is still error handling involved
/// // because this function returns an error if `x.width()` is larger than the target
/// // width.
/// x.sign_extend(width)?;
///
/// // Since error handling is involved no matter what, we have made the function
/// // signature accept `target_width: W` where
/// // `W: TryInto<BitWidth, Error = E>, crate::Error: From<E>`. This means that any
/// // type with an impl for `TryInto<BitWidth>` can be used as an argument. There is
/// // an `impl TryFrom<usize> for BitWidth`, so a plain `usize` can be entered into
/// // the function, and the function will handle both `BitWidth` invariant checking
/// // and its own invariants.
/// x.sign_extend(100)?;
///
/// Ok(x)
/// }
///
/// example_function(64).unwrap();
I will close this PR and open up some smaller ones based on the lessons I have learned
Changing the way
BitWidth
s are constructed is going to be a really invasive change no matter what. What this boils down to is:BitWidth
use aNonZeroUsize
internallyimpl From<usize> for BitWidth
. This was the last impl in this crate to have a hiddenunwrap
inside it. Thestd::ops
impls andbw
function haveunwrap
s inside them, but this is documented up front.impl From<NonZeroUsize> for BitWidth
impl TryFrom<usize> for BitWidth
bw
free function, aconst
function that takes ausize
and returns a validBitWidth
or panics. Hopefully, the neededconst
features will become stable as 1.45.0-nightly progresses. I could make it work on stable right now without the function beingconst
, but I wanted you to see what it should look like.BitWidth
with no generics. For example,pub fn zero_resize<W>(&mut self, target_width: W) where W: Into<BitWidth>
was changed intopub fn zero_resize(&mut self, target_width: BitWidth)
. We could maybe useW: TryInto<BitWidth>
for more ergonomics, but that means we have to change the return value to be aResult
.Result
because it can still fail), I change the signature fromW: Into<BitWidth>
to instead useW: TryInto<BitWidth, Error = E>, crate::Error: From<E>
. This is conceptually the same asW: TryInto<BitWidth>
, but the signature looks weird because I'm avoiding colliding with blanket impls, which would otherwise mean that plainBitWidth
s being entered into the function would not work. see https://github.com/rust-lang/rust/issues/69448 for more. Unfortunately, the intended ergonomics still do not work all the time (in some cases, you can see I had to use the turbofishes::<BitWidth, Infallible>
and::<usize, Error>
, I'm still not sure why this is required). Should I favor plainBitWidth
s instead?Digit::verify_valid_bitwidth
and theBitWidth::w*()
functions. There are slightly fewer lines needed, but vastly fewer actual characters needed when constructing lots ofBitWidth
sWhen we agree on the changes, I also want to do the same treatment for
BitPos
andShiftAmount
.