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

Clean up the associated constants #54

Merged
merged 2 commits into from
Feb 2, 2020

Conversation

AaronKutch
Copy link
Contributor

This makes constants such as digit::ZERO and digit::BITS into associated constants, which makes more sense and eliminates several imports for digit. I also remove redundant functions such as Digit::zero() and Digit::is_all_set(). I decided to keep Digit::is_zero() because of how often it is used and regularized all comparisons of Digit::ZERO.

@AaronKutch
Copy link
Contributor Author

One more important thing I should mention is that I added

pub(crate) use digit::{
    Digit,
    DoubleDigit,
};

to lib.rs. I believe this is a good change, because of how often these two imports are used.

@AaronKutch
Copy link
Contributor Author

Almost forgot to run cargo fmt

@coveralls
Copy link

coveralls commented Jan 1, 2020

Coverage Status

Coverage decreased (-0.9%) to 75.976% when pulling e7153d8 on AaronKutch:associated_constants into 21c4c2b on Robbepop:master.

@codecov-io
Copy link

codecov-io commented Jan 1, 2020

Codecov Report

Merging #54 into master will increase coverage by 0.42%.
The diff coverage is 96.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
+ Coverage   75.74%   76.16%   +0.42%     
==========================================
  Files          21       21              
  Lines        5363     5329      -34     
==========================================
- Hits         4062     4059       -3     
+ Misses       1301     1270      -31
Impacted Files Coverage Δ
src/radix.rs 88.88% <ø> (ø) ⬆️
src/apint/relational.rs 27.1% <0%> (+1.88%) ⬆️
src/apint/utils.rs 79.36% <0%> (ø) ⬆️
src/bitpos.rs 100% <100%> (ø) ⬆️
src/apint/shift.rs 100% <100%> (ø) ⬆️
src/digit.rs 94.74% <100%> (+1.72%) ⬆️
src/storage.rs 100% <100%> (ø) ⬆️
src/bitwidth.rs 98.59% <100%> (+0.02%) ⬆️
src/apint/bitwise.rs 82.63% <100%> (ø) ⬆️
src/apint/casting.rs 77.72% <100%> (+1.33%) ⬆️
... and 12 more

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 21c4c2b...e7153d8. Read the comment docs.

@Robbepop
Copy link
Owner

Robbepop commented Jan 1, 2020

I cleaned Travis CI caches and now it works again.

@AaronKutch
Copy link
Contributor Author

Is it easy to add a CI bot that checks that the crate is formatted?

@AaronKutch
Copy link
Contributor Author

Rust itself is trying to fix some associated constant related warts: rust-lang/rfcs#2700

@AaronKutch
Copy link
Contributor Author

I don't see anyway to make this more reviewable. This only changes the way Digit constants are imported and used, and does not change any functionality in functions. The only questionable part is

pub(crate) use digit::{
    Digit,
    DoubleDigit,
};

Previously in my crate reorganization suggestions, I had them used through an info module that shared various small structs. I think we want it to take it one step further by putting it at the crate root like this PR is currently doing, although we could change it in later reorganizations if we want to.

Other small structs that we expose to users are exposed at the crate root, and if we were to hypothetically expose Digit (probably never going to happen because I will have constructors and destructors with slices of u8 all the way to u128 anyway), it would be important enough to be at the crate root.

@AaronKutch
Copy link
Contributor Author

Its been one month. I am curious about how long it takes you to review PRs.

@Robbepop
Copy link
Owner

Robbepop commented Feb 2, 2020

Sorry I forgot about it. :S Will review in the course of today (sunday) or tomorrow.

Copy link
Owner

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, 1 question pls resolve

DataAccess::Ext(digits) => {
let (last, rest) = digits.split_last().unwrap_or_else(|| unreachable!());
last.is_one() && rest.iter().all(|digit| digit.is_zero())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I prefer the is_one version more because it is more readable imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I can keep is_one as a method for internal Digit use. Digit::ONE is used often enough that it could stay.

I am thinking about removing all is_one and creation of value 1 ApInts. The reason for this is the corner case where single bit signed interpretation ApInts can only represent negative one and zero. I thought about using this documentation:

    /// Returns `true` if the **unsigned** interpretation `ApInt` represents the value one (`1`).
    ///
    /// # Corner Case
    ///
    /// Normally, both signed and unsigned `ApInt`s have the same representation for the value one.
    /// However, the most significant and least significant bits are the same bit for
    /// `ApInt`s with a bitwidth of 1. This means that signed `ApInt`s with bitwidth 1
    /// can only represent negative one and zero. This function treats the
    /// `ApInt` as unsigned in this corner case.
    /// 
    /// # Note
    ///
    /// - One (`1`) is also called the multiplicative neutral element.
    /// - This operation is more efficient than comparing two instances of `ApInt`

The corner case problems stack up when considering what Int will do. Single bit ApInts with a set bit also fall under the more general case of ApInt::signed_min_value(). I think I should add some documentation on the ApInt struct or very high up about being careful with ApInt::signed_min_value() in algorithms and mentioning single bit ApInts.

I think that zero, unsigned_min_value, unsigned_max_value, signed_min_value, and signed_max_value are the only special numbers that get their own functions.

Should I remove all public facing special is_one now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the documentation I could give an example of the proper way to handle magic numbers.
For example, handling creation of 1 can be handled ApInt::unsigned_max_value(bitwidth 1).unsigned_extension_fn... or ApInt::from(123u8).extend... which forces the user to consider what happens with small bitwidth values

src/digit.rs Show resolved Hide resolved
@Robbepop Robbepop merged commit b8f4320 into Robbepop:master Feb 2, 2020
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

Successfully merging this pull request may close these issues.

4 participants